[gjs/wip/ptomato/develop: 3/4] coverage: Use Map objects for function counters

[Philip Chimento <pchimento src gnome org>]

commit bcc05f2665b6935d4380489cc3429bef8d5b659d
Author: Philip Chimento <philip chimento gmail com>
Date:   Sun Sep 24 23:25:30 2017 -0700

    coverage: Use Map objects for function counters
    
    We can hopefully make the function counters more efficient by using Map
    objects instead of plain objects, and since we now always have accurate
    info about the line where a function is defined, we can combine the
    function's name and line into one key, thereby only maintaining two
    levels of Map instead of three.
    
    This also refers to a function map key (name:line:nArgs) as a 'key'
    everywhere, and not a 'name'.

 gjs/coverage.cpp                   |    6 ++-
 installed-tests/js/testCoverage.js |   54 +++++++++++------------
 modules/_bootstrap/coverage.js     |   85 +++++++++++++++---------------------
 test/gjs-test-coverage.cpp         |    2 +-
 4 files changed, 66 insertions(+), 81 deletions(-)
---
diff --git a/gjs/coverage.cpp b/gjs/coverage.cpp
index d79fad3..c0de42b 100644
--- a/gjs/coverage.cpp
+++ b/gjs/coverage.cpp
@@ -548,8 +548,9 @@ convert_and_insert_function_decl(GArray         *array,
     JS::RootedObject object(context, &element.toObject());
     JS::RootedValue function_name_property_value(context);
 
-    if (!gjs_object_require_property(context, object, NULL, GJS_STRING_NAME,
-                                     &function_name_property_value))
+    JS::RootedId key_name(context, gjs_intern_string_to_id(context, "key"));
+    if (!gjs_object_require_property(context, object, "function element",
+                                     key_name, &function_name_property_value))
         return false;
 
     GjsAutoJSChar utf8_string(context);
@@ -991,6 +992,7 @@ gjs_get_file_checksum(GFile *file)
  *         } branch_info ] branches;
  *         array [ tuple {
  *             int line;
+ *             int n_params;
  *             string key;
  *         } function ] functions;
  *     } file ] files;
diff --git a/installed-tests/js/testCoverage.js b/installed-tests/js/testCoverage.js
index e1def31..e66319e 100644
--- a/installed-tests/js/testCoverage.js
+++ b/installed-tests/js/testCoverage.js
@@ -854,7 +854,7 @@ describe('Coverage', function () {
         let detectedFunctions = Coverage.functionsForAST(ast);
         let functionCounters =
             Coverage._functionsToFunctionCounters('script', detectedFunctions);
-        expect(functionCounters.name['1']['0'].hitCount).toEqual(0);
+        expect(functionCounters.get('name:1').get(0).hitCount).toEqual(0);
     });
 
     it('reports an error when two indistinguishable functions are present', function () {
@@ -869,26 +869,24 @@ describe('Coverage', function () {
     });
 
     it('converts function counters to an array', function () {
-        let functionsMap = {
-            '(anonymous)': {
-                '2': {
-                    '0': {
-                        hitCount: 1
-                    },
-                },
-            },
-            'name': {
-                '1': {
-                    '0': {
-                        hitCount: 0
-                    },
-                },
-            }
-        };
+        let functionsMap = new Map([
+            ['(anonymous):2', new Map([[0, {hitCount: 1}]])],
+            ['name:1', new Map([[0, {hitCount: 0}]])],
+        ]);
 
         let expectedFunctionCountersArray = [
-            jasmine.objectContaining({ name: '(anonymous):2:0', hitCount: 1 }),
-            jasmine.objectContaining({ name: 'name:1:0', hitCount: 0 })
+            {
+                key: '(anonymous):2:0',
+                line: 2,
+                nArgs: 0,
+                hitCount: 1,
+            },
+            {
+                key: 'name:1:0',
+                line: 1,
+                nArgs: 0,
+                hitCount: 0,
+            },
         ];
 
         let convertedFunctionCounters = Coverage._convertFunctionCountersToArray(functionsMap);
@@ -906,7 +904,7 @@ describe('Coverage.incrementFunctionCounters', function () {
         ]);
         Coverage._incrementFunctionCounters(functionCounters, 'f', 1, 0);
 
-        expect(functionCounters.f['1']['0'].hitCount).toEqual(1);
+        expect(functionCounters.get('f:1').get(0).hitCount).toEqual(1);
     });
 
     it('can disambiguate two functions with the same name', function () {
@@ -921,8 +919,8 @@ describe('Coverage.incrementFunctionCounters', function () {
         Coverage._incrementFunctionCounters(functionCounters, '(anonymous)', 1, 0);
         Coverage._incrementFunctionCounters(functionCounters, '(anonymous)', 2, 0);
 
-        expect(functionCounters['(anonymous)']['1']['0'].hitCount).toEqual(1);
-        expect(functionCounters['(anonymous)']['2']['0'].hitCount).toEqual(1);
+        expect(functionCounters.get('(anonymous):1').get(0).hitCount).toEqual(1);
+        expect(functionCounters.get('(anonymous):2').get(0).hitCount).toEqual(1);
     });
 
     it('can disambiguate two functions on same line with different params', function () {
@@ -937,8 +935,8 @@ describe('Coverage.incrementFunctionCounters', function () {
         Coverage._incrementFunctionCounters(functionCounters, '(anonymous)', 1, 0);
         Coverage._incrementFunctionCounters(functionCounters, '(anonymous)', 1, 1);
 
-        expect(functionCounters['(anonymous)']['1']['0'].hitCount).toEqual(1);
-        expect(functionCounters['(anonymous)']['1']['1'].hitCount).toEqual(1);
+        expect(functionCounters.get('(anonymous):1').get(0).hitCount).toEqual(1);
+        expect(functionCounters.get('(anonymous):1').get(1).hitCount).toEqual(1);
     });
 
     it('can disambiguate two functions on same line by guessing closest params', function () {
@@ -956,8 +954,8 @@ describe('Coverage.incrementFunctionCounters', function () {
          * latter function as opposed to the former. */
         Coverage._incrementFunctionCounters(functionCounters, '(anonymous)', 1, 2);
 
-        expect(functionCounters['(anonymous)']['1']['0'].hitCount).toEqual(0);
-        expect(functionCounters['(anonymous)']['1']['3'].hitCount).toEqual(1);
+        expect(functionCounters.get('(anonymous):1').get(0).hitCount).toEqual(0);
+        expect(functionCounters.get('(anonymous):1').get(3).hitCount).toEqual(1);
     });
 
     it('throws an error on unexpected function', function () {
@@ -1143,8 +1141,8 @@ describe('Coverage statistics container', function () {
 
         it('have same function keys as reflection', function () {
             /* Functions start on line 1 */
-            expect(Object.keys(statisticsWithNoCaching.functionCounters))
-                .toEqual(Object.keys(statistics.functionCounters));
+            expect([...statisticsWithNoCaching.functionCounters.keys()])
+                .toEqual([...statistics.functionCounters.keys()]);
         });
     });
 });
diff --git a/modules/_bootstrap/coverage.js b/modules/_bootstrap/coverage.js
index e83e25c..0c2ec6c 100644
--- a/modules/_bootstrap/coverage.js
+++ b/modules/_bootstrap/coverage.js
@@ -499,21 +499,23 @@ function _branchesToBranchCounters(branches, nLines) {
 }
 
 function _functionsToFunctionCounters(script, functions) {
-    let functionCounters = {};
+    let functionCounters = new Map();
 
     functions.forEach(function(func) {
-        let [name, line, args] = func.key.split(':');
+        let [name] = func.key.split(':', 1);
+        let {line, n_params} = func;
 
-        if (functionCounters[name] === undefined) {
-            functionCounters[name] = {};
-        }
-
-        if (functionCounters[name][line] === undefined) {
-            functionCounters[name][line] = {};
+        let argsMap;
+        let key = `${name}:${line}`;
+        if (functionCounters.has(key)) {
+            argsMap = functionCounters.get(key);
+        } else {
+            argsMap = new Map();
+            functionCounters.set(key, argsMap);
         }
 
-        if (functionCounters[name][line][args] === undefined) {
-            functionCounters[name][line][args] = { hitCount: 0 };
+        if (!argsMap.has(n_params)) {
+            argsMap.set(n_params, {hitCount: 0});
         } else {
             log(script + ':' + line + ' Function identified as ' +
                 func.key + ' already seen in this file. Function coverage ' +
@@ -524,24 +526,20 @@ function _functionsToFunctionCounters(script, functions) {
     return functionCounters;
 }
 
-function _identifyFunctionCounterInLinePartForDescription(linePart,
-                                                          nArgs) {
+function _identifyFunctionCounterInArgsMap(argsMap, nArgs) {
     /* There is only one potential option for this line. We might have been
      * called with the wrong number of arguments, but it doesn't matter. */
-    if (Object.getOwnPropertyNames(linePart).length === 1)
-        return linePart[Object.getOwnPropertyNames(linePart)[0]];
+    if (argsMap.size === 1)
+        return argsMap.values().next().value;
 
     /* Have to disambiguate using nArgs and we have an exact match. */
-    if (linePart[nArgs] !== undefined)
-        return linePart[nArgs];
+    if (argsMap.has(nArgs))
+        return argsMap.get(nArgs);
 
     /* There are multiple options on this line. Pick the one where
      * we satisfy the number of arguments exactly, or failing that,
      * go through each and pick the closest. */
-    let allNArgsOptions = Object.keys(linePart).map(function(key) {
-        return parseInt(key);
-    });
-
+    let allNArgsOptions = [...argsMap.keys()];
     let closest = allNArgsOptions.reduce(function(prev, current, index, array) {
         let nArgsOption = array[index];
         if (Math.abs(nArgsOption - nArgs) < Math.abs(current - nArgs)) {
@@ -551,26 +549,24 @@ function _identifyFunctionCounterInLinePartForDescription(linePart,
         return current;
     });
 
-    return linePart[String(closest)];
+    return argsMap.get(closest);
 }
 
 /**
  * _incrementFunctionCounters
  *
- * functionCounters: An object which is a key-value pair with the following schema:
- * {
- *      "key" : { line, hitCount }
- * }
+ * functionCounters: a Map object returned from _functionsToFunctionCounters()
  * name: The name of the function or "(anonymous)" if it has no name
  * line: The line at which this function is defined.
  * nArgs: The number of arguments this function has.
  */
 function _incrementFunctionCounters(functionCounters, name, line, nArgs) {
     let functionCountersForKey;
-    let linePart = functionCounters[name][line];
-    if (linePart) {
+    let key = `${name}:${line}`;
+    if (functionCounters.has(key)) {
+        let argsMap = functionCounters.get(key);
         functionCountersForKey =
-            _identifyFunctionCounterInLinePartForDescription(linePart, nArgs);
+            _identifyFunctionCounterInArgsMap(argsMap, nArgs);
     }
 
     if (functionCountersForKey) {
@@ -659,30 +655,23 @@ var _BranchTracker = class {
 
 function _convertFunctionCountersToArray(functionCounters) {
     let arrayReturn = [];
-    /* functionCounters is an object so explore it to create a
+    /* functionCounters is a map so iterate through it to create a
      * set of function keys and then convert it to
      * an array-of-object using the key as a property
      * of that object */
-    for (let name of Object.getOwnPropertyNames(functionCounters)) {
-        let namePart = functionCounters[name];
-
-        for (let line of Object.getOwnPropertyNames(namePart)) {
-            let linePart = functionCounters[name][line];
-
-            for (let nArgs of Object.getOwnPropertyNames(linePart)) {
-                let functionKey = [name, line, nArgs].join(':');
-                arrayReturn.push({ name: functionKey,
-                                   line: Number(line),
-                                   nArgs: nArgs,
-                                   hitCount: linePart[nArgs].hitCount });
-            }
+    for (let [nameLineKey, linePart] of functionCounters) {
+        let [name, line] = nameLineKey.split(':');
+        line = Number(line);
+        for (let [nArgs, {hitCount}] of linePart) {
+            let key = [name, line, nArgs].join(':');
+            arrayReturn.push({key, line, nArgs, hitCount});
         }
     }
 
     arrayReturn.sort(function(left, right) {
-        if (left.name < right.name)
+        if (left.key < right.key)
             return -1;
-        else if (left.name > right.name)
+        else if (left.key > right.key)
             return 1;
         else
             return 0;
@@ -785,12 +774,8 @@ var CoverageStatisticsContainer = class {
                 checksum: mtime === null ? getFileChecksum(filename) : null,
                 lines: [],
                 branches: [],
-                functions: 
_convertFunctionCountersToArray(statisticsForFilename.functionCounters).map(function(func) {
-                    return {
-                        key: func.name,
-                        line: func.line
-                    };
-                })
+                functions: _convertFunctionCountersToArray(statisticsForFilename.functionCounters)
+                    .map(({key, line, nArgs}) => ({key, line, n_params: nArgs})),
             };
 
             /* We're using a index based loop here since we need access to the
diff --git a/test/gjs-test-coverage.cpp b/test/gjs-test-coverage.cpp
index c7b2824..cb6fbf4 100644
--- a/test/gjs-test-coverage.cpp
+++ b/test/gjs-test-coverage.cpp
@@ -2303,7 +2303,7 @@ void gjs_test_add_tests_for_coverage()
             "resource://org/gnome/gjs/mock/test/gjs-test-coverage/cache_notation/simple_function.js",
             "1,2",
             "",
-            "\"key\":\"f:1:0\",\"line\":1"
+            "\"key\":\"f:1:0\",\"line\":1,\"n_params\":0"
         }
     };