-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Node v8 4-5x slower than previous versions #13445
Comments
I was about to try to look into it myself, but it appears the Have you tried running with the appropriate V8 flags to see if things are being deoptimized permanently or something similar? You can check for permanent deopts with:
Otherwise you can generally see what V8 is optimizing, deoptimizing, and/or inlining or not with the |
Here's the output of the first command.
I'll mess around with the others. |
Try fixing those permanent deopts first and see what kind of impact that has. For example, the ones about The others like 'Bad value context for arguments value' may not be as straightforward. That particular deopt typically has to do with having |
You can also check if your code heavily uses the patterns that are currently slower in the Node.js 8 in comparison with the previous versions: https://fhinkel.github.io/six-speed/ cc @nodejs/performance, @nodejs/v8 |
Can you try with |
I too have been seeing some slower performance on node 8.0.0 with one of my projects. In my case, I managed to narrow down the culprit and found that @milesj I peeked into your code and saw that one of your tests uses |
@charlieduong94 I cannot reproduce the Code and results (click me):console.log(`
// Node.js ${process.versions.node} (v8 ${process.versions.v8}) ${process.config.variables.target_arch}
`);
const str = 'ab'.repeat(1000);
let len;
console.time("'a'");
for (let i = 0; i < 1e4; i++) len = str.split('a').length;
console.timeEnd("'a'");
console.time('/a/');
for (let i = 0; i < 1e4; i++) len = str.split(/a/).length;
console.timeEnd('/a/');
console.time(" ''");
for (let i = 0; i < 1e4; i++) len = str.split('').length;
console.timeEnd(" ''");
process.title = len; // to not discard the results // Node.js 4.8.3 (v8 4.5.103.47) x64
'a': 365ms
/a/: 1307ms
'': 112ms
// Node.js 6.10.3 (v8 5.1.281.101) x64
'a': 349.567ms
/a/: 1339.276ms
'': 106.782ms
// Node.js 7.10.0 (v8 5.5.372.43) x64
'a': 370.449ms
/a/: 1352.863ms
'': 105.928ms
// Node.js 8.0.0-nightly201703249ff7ed23cd (v8 5.6.326.57) x64
'a': 356.759ms
/a/: 1200.888ms
'': 99.012ms
// Node.js 8.0.0-nightly2017050673d9c0f903 (v8 5.7.492.69) x64
'a': 368.424ms
/a/: 1234.220ms
'': 107.315ms
// Node.js 8.0.0 (v8 5.8.283.41) x64
'a': 384.050ms
/a/: 1287.733ms
'': 94.300ms
// Node.js 8.0.0-test201704119b43f9c487 (v8 5.9.0 (candidate)) x64
'a': 376.580ms
/a/: 1251.424ms
'': 102.169ms
// Node.js 8.0.0-pre (v8 6.0.153) x64
'a': 363.516ms
/a/: 1292.573ms
'': 104.215ms |
@charlieduong94 Can you share the snippet of code you used to benchmark that function? Also, which other node version did you compare with (besides node v8.0.0)? |
Here's the snippet. I used benchmark.js to run the function as many times as possible in quick succession. I compared against node 7.6.0. const { Suite } = require('benchmark')
const str = 'this/string/has/some/slashes/in/it'
const suite = new Suite()
suite
.add('split', () => {
return str.split('/')
})
.on('cycle', (event) => {
console.log(String(event.target))
})
.on('complete', function (event) {
console.log('Fastest is ' + this.filter('fastest').map('name'))
})
.run({ async: true }) |
I think I've verified this. Here's a simpler reproduction: const str = 'this/string/has/some/slashes/in/it';
const header = `split ${process.version}`;
const n = 1e7;
console.time(header);
for (var i = 0; i < n; ++i)
str.split('/');
console.timeEnd(header); Results:
|
Thanks guys for looking into this. I've been out of town the last few days and am just now getting back to this. I've ran both
The deopt output is far too large, but it was basically repeating this a lot.
|
@schuay can probably shed some light on this. |
Thanks for reporting, this looks like a bug in V8. I created http://crbug.com/v8/6463 to track this on our side. What's happening is that |
@schuay Can you explain why the results in these two tests differ? @vsemozhetbyt (no regression): #13445 (comment) @mscdex (regression): #13445 (comment) |
Your test does not cache results because the subject string isn't internalized, so it isn't affected by the bug above. (Internalized strings are deduplicated and stored in a hash table inside v8, so equality can be determined just by comparing pointers. E.g. string literals are turned into internalized strings.) |
@schuay Is there a way to forcibly internalize a non-literal string (e.g. mentioned |
@vsemozhetbyt You can run node with --allow-natives-syntax and then use
|
@schuay I have just stumbled upon this line in a benchmark (+another one). Is it somehow connected with string internalizing? |
@vsemozhetbyt no, flattening is another concept - it's intended to ensure that the string contents are in a plain sequential memory area (like C strings). |
The fix (http://crbug.com/v8/6463) has now been backmerged to V8 6.0, and should be picked up automatically for Node lts. |
Since the fix is trivial, it might be worth considering to cherry-pick this into Node master (which is now at 5.9). |
I'm +1 on cherry-picking this. |
I can do that |
@targos can we get this included in the V8 8.x backport? |
Or course. I will open a PR for master and include it in the backport next Monday. |
Awesome, thanks everyone! |
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{nodejs#45724} Fixes: nodejs#13445
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{#45724} Fixes: #13445 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{#45724} Fixes: #13445 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{#45724} Fixes: #13445 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{#45724} Fixes: #13445 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{#45724} Fixes: #13445 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{#45724} Fixes: #13445 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{nodejs#45724} Fixes: nodejs#13445
Original commit message: [string] Re-enable result caching for String.p.split Runtime::kStringSplit's result caching is only enabled when limit equals kMaxUInt32. BUG=v8:6463 Review-Url: https://codereview.chromium.org/2923183002 Cr-Commit-Position: refs/heads/master@{#45724} Fixes: #13445 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Occurs on both OSX and Linux (Travis CI).
I've noticed that one of my projects unit tests are running 4-5x slower on Node v8 than previous versions. I have yet to determine the cause, but an example can be found in the Travis builds: https://travis-ci.org/milesj/emoji-database
This is 100% reproducible and consistent, even on OSX. Here's an output of the tests being run on OSX (notice the times).
Compared to Node v7.
https://github.com/milesj/emoji-database
The text was updated successfully, but these errors were encountered: