Skip to content
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

Array Joining is slower in Node 6.x #9039

Closed
wldcordeiro opened this issue Oct 11, 2016 · 13 comments
Closed

Array Joining is slower in Node 6.x #9039

wldcordeiro opened this issue Oct 11, 2016 · 13 comments
Labels
performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.

Comments

@wldcordeiro
Copy link

wldcordeiro commented Oct 11, 2016

  • 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)
@Fishrock123 Fishrock123 added the performance Issues and PRs related to the performance of Node.js. label Oct 11, 2016
@wldcordeiro
Copy link
Author

Added Node v6.7.0 for posterity

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Oct 11, 2016
@mscdex
Copy link
Contributor

mscdex commented 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-inlining with each node version. FWIW I had to do these comparisons against a simple for-loop instead of using the benchmark module 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
Copy link
Contributor

@bmeurer is the fast path already landed?

@bmeurer
Copy link
Member

bmeurer commented Oct 12, 2016

Nope. There was some work on Array.prototype.join, but that is on hold due to other priorities.

@ronkorving
Copy link
Contributor

How does Node 7 fare?

@mscdex
Copy link
Contributor

mscdex commented 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:

function foo(a, b, c) {
  return [a,b,c].join(' ');
}

console.time('join');
for (var i=0; i<1e7; ++i)
  foo('1', '2', '3');
console.timeEnd('join');

Results:

node time (ms)
v4.5.0 392.500
v5.11.0 379.624
v6.5.0 1703.988
master 1601.958

@ofrobots
Copy link
Contributor

Trying to summarize the issue and find the path forward.. the issue is due to the removal of the hacky %_FastOneByteArrayJoin intrinsic 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.join at some point in the future. Given that Node.js v6.x has 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 %_FastOneByteArrayJoin in v6.x (@bmeurer: is this feasible?).

I am not sure if it is worth it though (counter-arguments welcome).

@ofrobots
Copy link
Contributor

/cc @nodejs/lts

@bmeurer
Copy link
Member

bmeurer commented 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.join to a proper TurboFan builtin (based on the CodeStubAssembler), 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
Copy link
Contributor

caitp commented 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
Copy link
Member

Trott commented Jul 15, 2017

Here's the current situation on my machine:

  • 8.1.4: Result: test-join-str x 3,279,041 ops/sec ±6.35% (81 runs sampled)
  • 6.7.0: Result: test-join-str x 3,484,331 ops/sec ±0.85% (85 runs sampled)
  • 5.12.0: Result: test-join-str x 7,309,367 ops/sec ±1.21% (86 runs sampled)

@caitp
Copy link
Contributor

caitp commented Jul 15, 2017

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.join to a proper TurboFan builtin (based on the CodeStubAssembler), 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?

This fell off my radar. If you throw me a design, I'll put some hours into it

@apapirovski
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

10 participants