Closed milesj closed 7 years ago
I was about to try to look into it myself, but it appears the emoji-database
module depends on at least a 148mb dependency (emojione
) that npm wants nothing to do with.
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:
node --trace-opt --trace-file-names myscript.js | grep -i 'aborted\|disabled'
Otherwise you can generally see what V8 is optimizing, deoptimizing, and/or inlining or not with the --trace-opt
, --trace-deopt
, and --trace-inlining
flags respectively.
Here's the output of the first command.
node --trace-opt --trace-file-names ./node_modules/.bin/run-tests ./tests/packageData.test.js | grep -i 'aborted\|disabled'
[aborted optimizing 0x27e1aedbba81 <JS Function </Users/Miles/Sites/emoji-database/node_modules/yargs/lib/argsert.js> (SharedFunctionInfo 0xecb8c886681)> because: Unsupported phi use of const or let variable]
[disabled optimization for 0xecb8c886681 <SharedFunctionInfo>, reason: Unsupported phi use of const or let variable]
[disabled optimization for 0x2b7b9f96ea89 <SharedFunctionInfo createShortnames>, reason: Unsupported let compound assignment]
[disabled optimization for 0x29ef9f818ac1 <SharedFunctionInfo it>, reason: Bad value context for arguments value]
[disabled optimization for 0x29ef9f815809 <SharedFunctionInfo>, reason: Bad value context for arguments value]
[aborted optimizing 0xcbf7ffb02c1 <JS Function toBeOneOf </Users/Miles/Sites/emoji-database/tests/setup.js> (SharedFunctionInfo 0x855beb486b9)> because: Unsupported phi use of const or let variable]
[disabled optimization for 0x855beb486b9 <SharedFunctionInfo toBeOneOf>, reason: Unsupported phi use of const or let variable]
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 let
or const
would involve changing variable declarations in those functions one by one back to var
until the deopt message disappears.
The others like 'Bad value context for arguments value' may not be as straightforward. That particular deopt typically has to do with having func.apply(..., arguments)
in functions that have properties tacked onto them. It's hard to say whether this is being done in the test runner itself, just in the tests, or in the actual module code.
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 --turbo
or alternatively using Node LKGR?
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 String.prototype.split
is approximately 5x slower now. I haven't tried benchmarking any of the other string methods to see if there were performance hits there.
@milesj I peeked into your code and saw that one of your tests uses split
quite frequently. I have a strong feeling that you are seeing the same performance issues I am.
@charlieduong94 I cannot reproduce the String.prototype.split
slowdown with a naive test:
@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 })
Here are the results I got:
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:
split v9.0.0-pre: 3546.705ms
split v7.9.0: 713.974ms
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 --trace-deopt
and --trace-inlining
, but I'm not exactly sure what I should be looking for. Here's the output.
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 String.p.split
has a fast path that caches results for certain inputs. A recent change had the unintended consequence of disabling this path.
@schuay Can you explain why the results in these two tests differ?
@vsemozhetbyt (no regression): https://github.com/nodejs/node/issues/13445#issuecomment-306358293
@mscdex (regression): https://github.com/nodejs/node/issues/13445#issuecomment-306382960
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 'ab'.repeat(1000)
), say, for a test purpose?
@vsemozhetbyt You can run node with --allow-natives-syntax and then use
%InternalizeString(str) // Internal function, subject to change at any time.
@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). cc @targos @MylesBorins
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!
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