Repository navigation
Regressions in buffer operations - 4.4.0 #38
Description
Hello,
One of the benchmarks we run contains what is in effect a number of microbenchmarks. We do a number of different actions with buffers, and I noticed in 4.4.0 we have seen a drop in some of these buffer tests.
One of these tests is looking at how long it takes to create a new buffer from an array of data.
var harness = require('./harness.js');
var ARRAY = [1, 2, 23829, 4, 5, 7, 12312321, 2131, 434832, 43792, 23421, 65345, 132210, 77777, 322131, 1, 2, 23829, 4, 5, 7, 12312321, 2131, 434832, 43792, 23421, 65345, 132210, 77777, 322131, 1, 2, 23829, 4, 5, 7, 12312321, 2131, 434832, 43792, 23421, 65345, 132210, 77777, 322131, 1, 2, 23829, 4, 5, 7, 12312321, 2131, 434832, 43792, 23421, 65345, 132210, 77777, 322131];
var ITERATIONS = 300000;
var result;
function test() {
for(var i=0;i<ITERATIONS;i++) {
result = new Buffer(ARRAY);
}
}
harness.run_test(test);Our harness repeats the test function a number of times, either until we get good consistency in scores, or until a max number of attempts. We then report a number of operations per second.
Comparing 4.3.2 and 4.4.0 we saw quite a drop in the ops/sec for this test. Looking at the changes between the builds, I saw that 4.4.0 had some changes to lib/buffer.js as per nodejs/node#4886 .
I rebuilt node 4.4.0 with the buffer.js from 4.3.2 and saw the performance recover:
4.4.0 with the buffer.js from 4.3.2:
sdk4.4.0_oldbuffer/bin/node new-buffer-from-array-test.js
total time: 5.093s -- iterations: 43 -- ops/sec: 8.44 -- average time: 0.12s -- variance: 2.19%
total time: 5.077s -- iterations: 43 -- ops/sec: 8.47 -- average time: 0.12s -- variance: 0.22%
total time: 5.076s -- iterations: 43 -- ops/sec: 8.47 -- average time: 0.12s -- variance: 0.32%
total time: 5.076s -- iterations: 43 -- ops/sec: 8.47 -- average time: 0.12s -- variance: 0.18%
4.4.0 unmodified
sdk4.40/bin/node new-buffer-from-array-test.js
total time: 5.003s -- iterations: 26 -- ops/sec: 5.2 -- average time: 0.19s -- variance: 4.42%
total time: 5.059s -- iterations: 26 -- ops/sec: 5.14 -- average time: 0.19s -- variance: 7.04%
total time: 5.149s -- iterations: 27 -- ops/sec: 5.24 -- average time: 0.19s -- variance: 0.24%
total time: 5.149s -- iterations: 27 -- ops/sec: 5.24 -- average time: 0.19s -- variance: 0.24%
Looking at the changes in buffer.js, they mainly seem to be changing var to either const or let.
I reverted these ones, and saw the performance improve:
4.4.0 with the const & let changes switched back to var
sdk4.4.0-testfix/bin/node new-buffer-from-array-test.js
total time: 5.062s -- iterations: 42 -- ops/sec: 8.3 -- average time: 0.12s -- variance: 2.11%
total time: 5.039s -- iterations: 42 -- ops/sec: 8.33 -- average time: 0.12s -- variance: 0.13%
total time: 5.045s -- iterations: 42 -- ops/sec: 8.33 -- average time: 0.12s -- variance: 1.09%
total time: 5.039s -- iterations: 42 -- ops/sec: 8.33 -- average time: 0.12s -- variance: 0.29%
Granted not quite to 100%, but much closer. Any ideas why this would cause a regression?
Activity
targos commentedon Mar 18, 2016
cc @nodejs/lts @Trott @trevnorris
@gareth-ellis
Could you make another try by only changing let to var here ?
gareth-ellis commentedon Mar 18, 2016
Looks good -
4.4.0
sdk4.40/bin/node new-buffer-from-array-test.js
total time: 5.032s -- iterations: 27 -- ops/sec: 5.37 -- average time: 0.19s -- variance: 4.58%
total time: 5.175s -- iterations: 28 -- ops/sec: 5.41 -- average time: 0.18s -- variance: 0.26%
total time: 5.172s -- iterations: 28 -- ops/sec: 5.41 -- average time: 0.18s -- variance: 0.25%
total time: 5.174s -- iterations: 28 -- ops/sec: 5.41 -- average time: 0.18s -- variance: 0.23%
4.4.0 with old buffer
sdk4.4.0_oldbuffer/bin/node new-buffer-from-array-test.js
total time: 5.058s -- iterations: 42 -- ops/sec: 8.3 -- average time: 0.12s -- variance: 2.05%
total time: 5.047s -- iterations: 42 -- ops/sec: 8.32 -- average time: 0.12s -- variance: 0.31%
total time: 5.044s -- iterations: 42 -- ops/sec: 8.33 -- average time: 0.12s -- variance: 0.25%
total time: 5.045s -- iterations: 42 -- ops/sec: 8.33 -- average time: 0.12s -- variance: 0.27%
4.4.0 with let changed to var as per request from @targos
sdk4.4.0_justlet/bin/node new-buffer-from-array-test.js
total time: 5.105s -- iterations: 43 -- ops/sec: 8.42 -- average time: 0.12s -- variance: 2.05%
total time: 5.09s -- iterations: 43 -- ops/sec: 8.45 -- average time: 0.12s -- variance: 0.41%
total time: 5.089s -- iterations: 43 -- ops/sec: 8.45 -- average time: 0.12s -- variance: 0.41%
total time: 5.089s -- iterations: 43 -- ops/sec: 8.45 -- average time: 0.12s -- variance: 0.41%
MylesBorins commentedon Mar 18, 2016
Is this due to the version of v8 that we are currently shipping with v4? would v5 be suffering a similar fate? /cc @ofrobots
Based on these bench marks I'd be open to doing a project wide
s/for(let/for(var/, at least for ltsthoughts?
/cc @jasnell
jasnell commentedon Mar 18, 2016
It's likely due to some point in time optimization quirk with the version of v8. We can likely get those
let/constuses reverted in v4, just need a PR opened against v4.x-staging to do so!ofrobots commentedon Mar 18, 2016
I think it would be best to avoid
letinforloops until TurboFan becomes the default optimizer. See also: https://groups.google.com/d/msg/v8-users/hsUrt4I2D98/QpvLs6aFAQAJ.MylesBorins commentedon Mar 18, 2016
@ofrobots when is the timeline for TurboFan becoming the default optimizer? Should we be updating master / v5 as well?
gareth-ellis commentedon Mar 18, 2016
Once we know where the PR should just be targeted, I can raise it
MylesBorins commentedon Mar 18, 2016
@gareth-ellis I think that this should likely be targeted at master if TurboFan is not coming with v8 v5.x
If we do target master we will likely want to include a rule for eslint to enforce that we don't regress here
trevnorris commentedon Mar 18, 2016
v4 is when we switched to using the typed array API. I believe the mechanics are getting faster since then, but there was a known performance drop when moving over. Nothing we can do since the API previously relied on was removed.
gareth-ellis commentedon Mar 24, 2016
I think can close this too, the PR has landed