performance: changed ArrayPrototypeJoin with custom join implementation

[marco-ippolito]

As discussed in the last performance team meeting led by @anonrig, I've updated the primordial ArrayPrototypeJoin with a custom implementation because it is faster than the built-in in v8 6.0

[anonrig]

@marco-ippolito Can you remove the join implementation from internal/util.js and replace all calls to join with ArrayPrototypeJoin?

[anonrig]

CC @nodejs/performance

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48269/

[mcollina]

I'm not sure overriding primordials in this way is in line with the intent of primordials.

[anonrig]

I'm not sure overriding primordials in this way is in line with the intent of primordials.

@mcollina v8 team has no intention of fixing this performance issue (referencing: https://bugs.chromium.org/p/v8/issues/detail?id=7420&q=array.prototype.join&can=2). I believe this is beneficial in all places of Node.

[cjihrig]

How exactly is this implementation different from the real Array.prototype.join()? Are there edge cases in the spec that could come back to bite us by doing this?

[Ethan-Arrowood]

@cjihrig brings up a good point. If we are going to replace a primordial it needs to cover any edge case stipulated by the spec: https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array.prototype.join

For example, i think this incorrectly joins values null and undefined.

❯ node
Welcome to Node.js v18.12.1.
Type ".help" for more information.
> function join(output, separator = ',') {
...   let str = '';
...   if (output.length !== 0) {
...     const lastIndex = output.length - 1;
...     for (let i = 0; i < lastIndex; i++) {
...       // It is faster not to use a template string here
...       str += output[i];
...       str += separator;
...     }
...     str += output[lastIndex];
...   }
...   return str;
... }
undefined
> join(['a', undefined, 'b'])
'a,undefined,b'
> ['a', undefined, 'b'].join()
'a,,b'

[marco-ippolito]

@cjihrig brings up a good point. If we are going to replace a primordial it needs to cover any edge case stipulated by the spec: https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array.prototype.join

For example, i think this incorrectly joins values null and undefined.

❯ node
Welcome to Node.js v18.12.1.
Type ".help" for more information.
> function join(output, separator = ',') {
...   let str = '';
...   if (output.length !== 0) {
...     const lastIndex = output.length - 1;
...     for (let i = 0; i < lastIndex; i++) {
...       // It is faster not to use a template string here
...       str += output[i];
...       str += separator;
...     }
...     str += output[lastIndex];
...   }
...   return str;
... }
undefined
> join(['a', undefined, 'b'])
'a,undefined,b'
> ['a', undefined, 'b'].join()
'a,,b'

I'll create a test to cover all edge cases

[cjihrig]

I'll create a test to cover all edge cases

Or...... we could just use the real function. Performance isn't everything.

[aduh95]

I also don't think overriding primordials is the right way, it goes against the idea of primordials (to always be the guaranteed primordial value). You could create a FastArrayPrototypeJoin and add a lint rule to enforce its use over ArrayPrototypeJoin, not sure it's worth it outside fast paths though.

[tniessen]

Another concern is that we'll have to benchmark this with every V8 update to make sure there is still justification for the added complexity. Changes within V8 can affect performance patterns significantly.

Also, I assume that the performance gain is approximately linear in the length of the array, so the effect is likely only noticeable when joining many array elements into strings. In what hot paths do we do that?

[Ethan-Arrowood]

IMO the effort here is justified. Improvements like this can lead to an overall faster core which i am in favor of.

I like @aduh95 's idea of keeping the primordial as is and introducing a Fast version of it that folks can use when appropriate

[anonrig]

Another concern is that we'll have to benchmark this with every V8 update to make sure there is still justification for the added complexity. Changes within V8 can affect performance patterns significantly.

Also, I assume that the performance gain is approximately linear in the length of the array, so the effect is likely only noticeable when joining many array elements into strings. In what hot paths do we do that?

Here are the benchmarks with a variety of input sizes

• Empty array

╔══════════════════════╤═════════╤════════════════════╤═══════════╗
║ Slower tests         │ Samples │             Result │ Tolerance ║
╟──────────────────────┼─────────┼────────────────────┼───────────╢
║ Array.prototype.join │   10000 │ 11641118.29 op/sec │  ± 6.68 % ║
╟──────────────────────┼─────────┼────────────────────┼───────────╢
║ Fastest test         │ Samples │             Result │ Tolerance ║
╟──────────────────────┼─────────┼────────────────────┼───────────╢
║ fast-array-join      │   10000 │ 11722944.62 op/sec │  ± 1.76 % ║
╚══════════════════════╧═════════╧════════════════════╧═══════════╝

• Single element array (const singleArray = ["a"];)

╔══════════════════════╤═════════╤════════════════════╤═══════════╗
║ Slower tests         │ Samples │             Result │ Tolerance ║
╟──────────────────────┼─────────┼────────────────────┼───────────╢
║ Array.prototype.join │   10000 │  4148830.15 op/sec │ ± 38.38 % ║
╟──────────────────────┼─────────┼────────────────────┼───────────╢
║ Fastest test         │ Samples │             Result │ Tolerance ║
╟──────────────────────┼─────────┼────────────────────┼───────────╢
║ fast-array-join      │   10000 │ 11390468.68 op/sec │ ±  4.47 % ║
╚══════════════════════╧═════════╧════════════════════╧═══════════╝

• Short array (const shortArray = "abc".split("");)

╔══════════════════════╤═════════╤═══════════════════╤═══════════╗
║ Slower tests         │ Samples │            Result │ Tolerance ║
╟──────────────────────┼─────────┼───────────────────┼───────────╢
║ Array.prototype.join │   10000 │ 4559859.56 op/sec │ ± 32.31 % ║
╟──────────────────────┼─────────┼───────────────────┼───────────╢
║ Fastest test         │ Samples │            Result │ Tolerance ║
╟──────────────────────┼─────────┼───────────────────┼───────────╢
║ fast-array-join      │   10000 │ 5424720.34 op/sec │ ± 43.03 % ║
╚══════════════════════╧═════════╧═══════════════════╧═══════════╝

• Large array (const largeArray = "abcdefgh".repeat(100).split("");)

╔══════════════════════╤═════════╤══════════════════╤═══════════╗
║ Slower tests         │ Samples │           Result │ Tolerance ║
╟──────────────────────┼─────────┼──────────────────┼───────────╢
║ Array.prototype.join │    9500 │  95463.09 op/sec │  ± 0.97 % ║
╟──────────────────────┼─────────┼──────────────────┼───────────╢
║ Fastest test         │ Samples │           Result │ Tolerance ║
╟──────────────────────┼─────────┼──────────────────┼───────────╢
║ fast-array-join      │   10000 │ 155965.52 op/sec │  ± 2.00 % ║
╚══════════════════════╧═════════╧══════════════════╧═══════════╝

[tniessen]

Those numbers correspond, per operation, to saving 0.6 nanoseconds for empty arrays, 153 nanoseconds for one-element arrays, 34 nanoseconds for short arrays, and 4 microseconds for very large arrays.

So my question remains the same as in my previous comment: in what hot paths do we join arrays such that saving a few CPU cycles will cause the overall performance to improve significantly?

[mscdex]

None of the hotter paths where join is used showed any change in the relevant benchmarks with these changes.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48283/

[benjamingr]

I don't understand the discussion about primordials, my understnading is:

• There is a faster implementation we have for .join.
• We want to use it for performance gains

Why don't we just call our implementation in performance sensitive places without overriding the primordial?

[tniessen]

@benjamingr What you are describing is essentially the status quo. The slightly faster variant is only used by util.inspect(), I believe, which may or may not be a performance-sensitive place given that it is meant for debugging purposes. If there are other code paths that would see a statistically significant performance benefit, we could use it there as well, but my question regarding such code paths has thus far been unanswered.

[mcollina]

Making my objection to this explicit.

[marco-ippolito]

It's clear that the performance gains from this PR are not worth the trouble, considering this could change with any update of v8, for this reason I'm closing this.