Skip inherited properties in synthetic namespaces

[lukastaegert]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
Resolves #4245

Description

When merging namespaces, the "in" operator will also iterate over non-inherited namespaces, which may be a problem for synthetic namespaces (or external ones, depending on interop settings). This adds a very simple fix by checking if Object.getOwnPropertyDescriptor returns a non-falsy value, which essentially will skip inherited properties.

[github-actions]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#synthetic-namespace-inherited-props

or load it into the REPL:
https://rollupjs.org/repl/?pr=4256

[codecov]

Codecov Report

Merging #4256 (53b56b5) into master (73c10cd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4256   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         204      204           
  Lines        7283     7288    +5     
  Branches     2079     2081    +2     
=======================================
+ Hits         7166     7171    +5     
  Misses         58       58           
  Partials       59       59           

Impacted Files	Coverage Δ
src/utils/interopHelpers.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73c10cd...53b56b5. Read the comment docs.

[kzc]

I just applied the tests from this PR without the src changes and ran npm test to see what would happen. Only one test failed - synthetic-named-exports-fallback-es2015. Does the original bug only affect the --generatedCode es2015 scenario?

Not specific to this PR - when looking at the relative sizes of the various es5 and es2015 generated helpers I'm not seeing much difference between the two. Is it worth the extra code complexity to manage two sets of generated helpers?

[lukastaegert]

Yes it only affects he ES2015 one. The advantage of the ES2015 one is that is is faster as .forEach is a rather slow kind of loop while for in is much faster. But it relies on block-scoping.

[kzc]

V8 has optimized .forEach such that there's not much difference compared to a loop. I appreciate that for of would be a gain for es2015, but for in works fine in es5 with var variables. As far as I can tell no helper really needs for-loop scoped let/const variables - it's all synchronous in memory stuff going on. Then the two sets of helpers could be unified to simplify maintenance and reduce complexity.

[lukastaegert]

Well, use a var variable as loop index and access it asynchronously inside a getter to understand what I mean. We need the block scoping for the getters.

[kzc]

Here's a micro benchmark comparing the timings of a million calls to different implementations of _mergeNamespaces:

• .forEach,
• for in using const,
• and an ES5-compatible for in using var with an extra closure for the getter to achieve the same behavior.

$ node-v16.1.0 ns-bench.js
   real 6.87   user 8.81   sys 0.65   _mergeNamespaces_forEach
   real 6.83   user 8.03   sys 0.46   _mergeNamespaces_for_in_const
   real 6.81   user 7.88   sys 0.48   _mergeNamespaces_for_in_var

It's notable that the wall clock timings for the three implementations are essentially the same, only the user time for forEach is 10% higher.

$ cat ns-bench.js
var assert = require("assert");

function _mergeNamespaces_forEach(n, m) {
    m.forEach(function (e) {
        e && typeof e !== 'string' && !Array.isArray(e) && Object.keys(e).forEach(function (k) {
            if (k !== 'default' && !(k in n)) {
                var d = Object.getOwnPropertyDescriptor(e, k);
                Object.defineProperty(n, k, d.get ? d : {
                    enumerable: true,
                    get: function () { return e[k]; }
                });
            }
        });
    });
    return Object.freeze(n);
}

function _mergeNamespaces_for_in_const(n, m) {
    for (let i = 0; i < m.length; i++) {
        const e = m[i];
        if (typeof e !== 'string' && !Array.isArray(e)) { for (const k in e) {
            if (k !== 'default' && !(k in n)) {
                const d = Object.getOwnPropertyDescriptor(e, k);
                if (d) {
                    Object.defineProperty(n, k, d.get ? d : {
                        enumerable: true,
                        get: () => e[k]
                    });
                }
            }
        } }
    }
    return Object.freeze(n);
}

function _mergeNamespaces_for_in_var(n, m) {
    function _propGetter(o, k) { return function(){ return o[k]; } }
    for (var i = 0; i < m.length; i++) {
        var e = m[i];
        if (typeof e !== 'string' && !Array.isArray(e)) { for (var k in e) {
            if (k !== 'default' && !(k in n)) {
                var d = Object.getOwnPropertyDescriptor(e, k);
                if (d) {
                    Object.defineProperty(n, k, d.get ? d : {
                        enumerable: true,
                        get: _propGetter(e, k)
                    });
                }
            }
        } }
    }
    return Object.freeze(n);
}

function bench(algo) {
    var o = {
        a: 1,
        b: 22,
        c: 333,
        get d() { return 4444 },
        get e() { return 55555 },
        get f() { return 666666 },
    };

    var result = algo({}, [o]);
    o.b *= -1000; // demonstrate effect of changing input after merge result

    var s = "";
    s += JSON.stringify(result) + "\n";

    for (var len = 0, i = 0; i < 1e6; ++i)
        len += JSON.stringify(algo({}, [o])).length;

    return s + len;
}

function time(algo) {
    var realStart = Date.now();
    var usage = process.cpuUsage();
    var result = bench(algo);
    usage = process.cpuUsage(usage);
    var realEnd = Date.now();
    console.log("   real %s   user %s   sys %s   %s",
        ((realEnd - realStart) / 1000).toFixed(2),
        (usage.user / 1e6).toFixed(2),
        (usage.system / 1e6).toFixed(2),
        algo.name);
    return result;
}

switch (process.argv[2]) {
  case "-h":
    console.error("usage:", process.argv[1], "[forEach | for_in_const | for_in_var]");
    break;
  case "forEach":
    console.log(bench(_mergeNamespaces_forEach));
    break;
  case "for_in_const":
    console.log(bench(_mergeNamespaces_for_in_const));
    break;
  case "for_in_var":
    console.log(bench(_mergeNamespaces_for_in_var));
    break;
  default:
    var r1 = time(_mergeNamespaces_forEach);
    var r2 = time(_mergeNamespaces_for_in_const);
    var r3 = time(_mergeNamespaces_for_in_var);
    assert.strictEqual(r1, r2);
    assert.strictEqual(r2, r3);
    break;
}

$ /usr/bin/time node ns-bench.js forEach
{"a":1,"b":-22000,"c":333,"d":4444,"e":55555,"f":666666}
56000000
        6.99 real         8.87 user         0.69 sys

$ /usr/bin/time node ns-bench.js for_in_const
{"a":1,"b":-22000,"c":333,"d":4444,"e":55555,"f":666666}
56000000
        6.94 real         8.10 user         0.52 sys

$ /usr/bin/time node ns-bench.js for_in_var
{"a":1,"b":-22000,"c":333,"d":4444,"e":55555,"f":666666}
56000000
        6.88 real         7.87 user         0.54 sys

[lukastaegert]

I would expect the differences to be negligible if there are very few exports as then, the bench would be dominated by other factors. Also, millions of calls would be unrealistic as that would make the loop "hot" and cause the optimizing compiler to kick in.
Usually, mergeNamespaces is only called very few times in the beginning so it is unlikely to be optimized. On the other hand, optimizing for large namespaces is what the algorithm is aiming for.
Would be interesting if you could use an object with many exports but only run a few times.

[kzc]

I appreciate the bench is overkill for the task at hand. Whether it's run hot or cold it'll never process a great amount of data in real use. Object.defineProperty is doing the heavy lifting in all versions. Just wanted to demonstrate that the ES5 forEach was competitive with the es2015 version, and a performant ES5 compatible for-in-var solution also exists to unify the helpers and reduce complexity. I don't plan to work on it further. Feel free to ignore.

Side note: If you run an older version of node you can see how well the V8 team optimized forEach in recent versions to reach near parity with the for-in loop:

$ node-v6.9.0 ns-bench.js
   real 12.86   user 12.69   sys 0.26   _mergeNamespaces_forEach
   real 10.27   user 10.24   sys 0.08   _mergeNamespaces_for_in_const
   real 10.30   user 10.28   sys 0.07   _mergeNamespaces_for_in_var

$ node-v16.1.0 ns-bench.js
   real 6.87   user 8.81   sys 0.65   _mergeNamespaces_forEach
   real 6.83   user 8.03   sys 0.46   _mergeNamespaces_for_in_const
   real 6.81   user 7.88   sys 0.48   _mergeNamespaces_for_in_var

[kzc]

This is kind of interesting. Found a way to disable JIT in V8...

$ node-v16.1.0 --jitless --no-opt ns-bench.js 
   real 8.54   user 10.54   sys 0.62   _mergeNamespaces_forEach
   real 8.86   user 10.07   sys 0.45   _mergeNamespaces_for_in_const
   real 8.24   user 9.32   sys 0.46   _mergeNamespaces_for_in_var

It appears that block scoped variables (const / let) in loops impose a slight but noticeable cost compared to var variables at runtime in non-JIT mode. I'd guess it would be due to TDZ tracking overhead.