Repository navigation
Array Joining is slower in Node 6.x #9039
Description
- Versions: Tested in Node 5.9, 5.11, 6.0, 6.4, 6.5, 6.6, 6.7
- Platform: Linux (Kubuntu 16.04)
- Subsystem: N/A / Not sure?
It appears that joining together raw strings has some performance regressions in the 6.x line. I first noticed this when evaluating the classnames library for a project and running its benchmarks as my coworker on 5.9.0 was getting roughly double the speed I was getting with 6.6.0. But I've extracted the meat of the issue into a small sample test.
When I asked on #node-dev about this I was suggested to test 5.11 -> 6.0 as well as 6.4 -> 6.5 as those were major bumps for V8 versions.
The test requires benchmark.js, if someone could point me as to how to do a raw node benchmark this would be appreciated.
function join_strings() {
var result = [];
for (var i = 0; i < arguments.length; i++) {
if (typeof arguments[i] === 'string' || typeof arguments[i] === 'number') {
result.push(arguments[i]);
}
}
return result.join(' ');
}
// Test case.
var testVal = ['one', 'two', 'three'];
var benchmark = require('benchmark');
var suite = new benchmark.Suite();
suite.add('test-join-str', function() {
join_strings.apply(null, testVal);
});
suite.on('cycle', function(event) {
console.log('Result:', String(event.target));
});
suite.run();Results of tests
- Node 5.11.0 -
node min-string-test.js Result: test-join-str x 8,037,676 ops/sec ±1.05% (92 runs sampled) - Node 6.0.0 -
node min-string-test.js Result: test-join-str x 1,985,858 ops/sec ±1.71% (90 runs sampled) - Node 6.4.0 -
node min-string-test.js Result: test-join-str x 2,120,274 ops/sec ±0.93% (94 runs sampled) - Node 6.5.0 -
node min-string-test.js Result: test-join-str x 3,738,101 ops/sec ±0.92% (92 runs sampled) - Node 6.6.0 -
node min-string-test.js Result: test-join-str x 3,710,874 ops/sec ±1.45% (92 runs sampled) - Node 6.7.0 -
node min-string-test.js Result: test-join-str x 3,703,040 ops/sec ±0.98% (93 runs sampled)
Activity
wldcordeiro commentedon Oct 11, 2016
Added Node v6.7.0 for posterity
mscdex commentedon Oct 11, 2016
Confirmed. I'm willing to bet this change is the culprit based on the comparison of output of
--trace-opt --trace-deopt --trace-inliningwith each node version. FWIW I had to do these comparisons against a simple for-loop instead of using thebenchmarkmodule because the latter was spewing constant deopts and was much harder to read, however the end results were the same (node versions pre-v6 were faster)./cc @nodejs/v8 ?
jeisinger commentedon Oct 12, 2016
@bmeurer is the fast path already landed?
bmeurer commentedon Oct 12, 2016
Nope. There was some work on Array.prototype.join, but that is on hold due to other priorities.
ronkorving commentedon Oct 17, 2016
How does Node 7 fare?
mscdex commentedon Oct 17, 2016
@ronkorving I'm currently seeing master be ~6% faster than node v6 with a generic string join benchmark. FWIW here's the simpler benchmark I'm using:
Results:
ofrobots commentedon Oct 18, 2016
Trying to summarize the issue and find the path forward.. the issue is due to the removal of the hacky
%_FastOneByteArrayJoinintrinsic in V8 5.1. This was expected to affect microbenchmarks. Real world applications shouldn't have a significant impact as a result of this change.The V8 team has plans to implement a proper fast path for
Array.prototype.joinat some point in the future. Given that Node.jsv6.xhas rolled over to LTS now, it is not clear that, once the performance fix is available in upstream V8, it would be worth back-porting it to LTS. It would be neither a bug nor a security fix, and real world Node.js application performance is not expected to be affected significantly. I don't think this is a viable option.The other alternatively would be to revert the removal of
%_FastOneByteArrayJoininv6.x(@bmeurer: is this feasible?).I am not sure if it is worth it though (counter-arguments welcome).
ofrobots commentedon Oct 18, 2016
/cc @nodejs/lts
bmeurer commentedon Oct 18, 2016
In theory it'd be possible to reintroduce
%_FastOneByteArrayJoin, however that blocks optimization of the containing function and you need an appropriate bailout mechanism for TurboFan as well. I don't think this will help you a lot.The long-term solution is to port all of
Array.prototype.jointo a proper TurboFan builtin (based on theCodeStubAssembler), which also fixes a couple of other regressions that were introduced recently when we had to fix some Intl bugs.Right now we don't have the bandwidth to work on the long-term vision for
Array.prototype.join; we can help with the design and provide guidance on the implementation. Performance of join was always an issue./cc @caitp Is this maybe something you'd be interested in?
caitp commentedon Oct 18, 2016
I may be able to work on that, but if there isn't evidence that this has an impact on real applications, then maybe it could wait until 2017?
Trott commentedon Jul 15, 2017
Here's the current situation on my machine:
caitp commentedon Jul 15, 2017
This fell off my radar. If you throw me a design, I'll put some hours into it
apapirovski commentedon Mar 2, 2018
There has been no movement on this for a long time and it's a V8 issue, as such I'm going to go ahead and close this. Feel free to re-open if you feel I'm mistaken.