New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip inherited properties in synthetic namespaces #4256
Conversation
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: |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
I just applied the tests from this PR without the 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? |
Yes it only affects he ES2015 one. The advantage of the ES2015 one is that is is faster as |
V8 has optimized |
Well, use a |
Here's a micro benchmark comparing the timings of a million calls to different implementations of
It's notable that the wall clock timings for the three implementations are essentially the same, only the user time for $ 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;
}
|
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. |
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. Side note: If you run an older version of node you can see how well the V8 team optimized
|
This is kind of interesting. Found a way to disable JIT in V8...
It appears that block scoped variables (const / let) in loops impose a slight but noticeable cost compared to |
This PR contains:
Are tests included?
Breaking Changes?
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.