lib: add bound apply variants of varargs primordials

[ExE-Boss]

This removes the need for iterator destructuring of variable arguments for ArrayPrototypePush by adding bound apply variants of varargs primordials.

---

Refs: #36740 (comment), which inspired me to do this.

[aduh95]

Could you please add a comment like the uncurryThis one which explains this is equivalent to

const applyBind = (func) => (thisArg, args) => ReflectApply(func, thisArg, args);

[ExE-Boss]

Actually, it’s closer to:

const applyBind = (func, ...rest) => {
	if (args.length > 0) {
		const thisArg = rest[0];
		return (args = []) => ReflectApply(func, thisArg, args);
	} else {
		return (thisArg, args = []) => ReflectApply(func, thisArg, args);
	}
}

[aduh95]

BTW, shouldn't we prefer using Reflect.apply instead of Function.prototype.apply? I think if someone calls those methods without a argument array, it's almost certainly a bug, and using Reflect.apply would help spot that. Or is there a performance reason to use one or the other?

[ExE-Boss]

Well, Reflect.apply isn’t very optimised in comparison to bound Function.prototype.call and Function.prototype.apply calls.

Also, engines are able to generate better code for native bound functions compared to arrow functions that replicate the behaviour of a bound function.

[aduh95]

I meant to use const { apply } = Reflect compared to const { apply } = Function.prototype.

[ExE-Boss]

That wouldn’t work, since then applyBind(foo) would create a function that roughly does: (...args) => Reflect.apply.call(foo, ...args), instead of (...args) => Reflect.apply(foo, ...args).

[aduh95]

Gotcha, the equivalent would have to be:

Suggested change

	const applyBind = bind.bind(apply);
	const applyBind = bind.bind(Reflect.apply, null);

Reflect.apply isn’t very optimised in comparison to bound Function.prototype.call and Function.prototype.apply calls.

Is that generally true statement or is it specific to V8? When reading the spec, Reflect.apply seems to be doing less things than Function.prototype.apply, that's a bit counter-intuitive.

[ExE-Boss]

Reflect.apply isn’t very optimised in comparison to bound Function.prototype.call and Function.prototype.apply calls.

Is that generally true statement or is it specific to V8?

That’s mostly specific to V8, but I assume that it also applies to other C++-based engines, since V8 hasn’t put as much effort into optimising Reflect as into optimising Object and Function methods.

Refs: #36740 (comment)

[aduh95]

Could you also remove the use of ...new SafeArrayIterator when applicable? SafeArrayIterator has pretty poor performance.

node/lib/internal/http2/compat.js

Lines 152 to 153 in 900c9f5

	ArrayPrototypePush(request[kRawTrailers],
	...new SafeArrayIterator(rawTrailers));

node/lib/internal/worker.js

Lines 217 to 218 in 900c9f5

	ArrayPrototypePush(transferList,
	...new SafeArrayIterator(options.transferList));

[aduh95]

Actually, it seems the implementation is buggy, thisArg should be the first argument provided when calling primordials.…Apply, not src which references the prototype.

Maybe adding a test to ensure the implementation works would help:

const assert = require("assert")

const { primordials: {ArrayPrototypePushApply} }= require('internal/test/binding')

const arr1 = [1,2,3]
const arr2 = [4,5,6]

ArrayPrototypePushApply(arr1, arr2)
assert.deepStrictEqual(arr1, [1,2,3,4,5,6])

[Trott]

LGTM. What do we think of running some fairly wide-ranging benchmarks? Or at least re-running the benchmarks we've run since some changes to the code have happened?

Not for this PR, but as the primordials grow in usage and variety, we probably should make sure they are sufficiently documented.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35796/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/912/

[aduh95]

Benchmark CI results still look good.

                                                                     confidence improvement accuracy (*)    (**)   (***)
util/inspect-array.jstype='denseArray' len=100000 n=500                             -2.06 %       ±5.65%  ±7.53%  ±9.82%
util/inspect-array.jstype='denseArray' len=100 n=500                                -3.04 %       ±5.67%  ±7.54%  ±9.81%
util/inspect-array.jstype='denseArray_showHidden' len=100000 n=500                   1.20 %       ±4.58%  ±6.10%  ±7.94%
util/inspect-array.jstype='denseArray_showHidden' len=100 n=500                     -3.53 %       ±5.86%  ±7.81% ±10.18%
util/inspect-array.jstype='mixedArray' len=100000 n=500                              0.54 %       ±1.81%  ±2.41%  ±3.14%
util/inspect-array.jstype='mixedArray' len=100 n=500                                 0.36 %       ±4.65%  ±6.19%  ±8.06%
util/inspect-array.jstype='sparseArray' len=100000 n=500                             0.58 %       ±5.05%  ±6.76%  ±8.85%
util/inspect-array.jstype='sparseArray' len=100 n=500                               -1.70 %       ±6.77%  ±9.01% ±11.74%
util/inspect.jsoption='colors' method='Array' n=20000                               -0.04 %       ±1.93%  ±2.58%  ±3.36%
util/inspect.jsoption='colors' method='Date' n=20000                                -0.05 %       ±5.20%  ±6.92%  ±9.02%
util/inspect.jsoption='colors' method='Error' n=20000                               -1.18 %       ±4.00%  ±5.33%  ±6.94%
util/inspect.jsoption='colors' method='Number' n=20000                               0.85 %       ±3.68%  ±4.91%  ±6.41%
util/inspect.jsoption='colors' method='Object_deep_ln' n=20000                       1.02 %       ±2.70%  ±3.59%  ±4.69%
util/inspect.jsoption='colors' method='Object_empty' n=20000                         1.45 %       ±4.68%  ±6.22%  ±8.10%
util/inspect.jsoption='colors' method='Object' n=20000                               0.85 %       ±3.04%  ±4.04%  ±5.27%
util/inspect.jsoption='colors' method='Set' n=20000                                 -0.65 %       ±4.53%  ±6.02%  ±7.84%
util/inspect.jsoption='colors' method='String_boxed' n=20000                         0.12 %       ±4.08%  ±5.43%  ±7.07%
util/inspect.jsoption='colors' method='String_complex' n=20000                      -0.32 %       ±5.25%  ±6.99%  ±9.10%
util/inspect.jsoption='colors' method='String' n=20000                               4.48 %       ±5.82%  ±7.76% ±10.12%
util/inspect.jsoption='colors' method='TypedArray_extra' n=20000                    -0.38 %       ±2.05%  ±2.73%  ±3.56%
util/inspect.jsoption='colors' method='TypedArray' n=20000                          -1.43 %       ±2.60%  ±3.45%  ±4.50%
util/inspect.jsoption='none' method='Array' n=20000                                  2.95 %       ±7.91% ±10.53% ±13.72%
util/inspect.jsoption='none' method='Date' n=20000                                  -2.00 %       ±5.56%  ±7.42%  ±9.69%
util/inspect.jsoption='none' method='Error' n=20000                                 -1.89 %       ±4.54%  ±6.04%  ±7.88%
util/inspect.jsoption='none' method='Number' n=20000                                 2.12 %       ±3.29%  ±4.37%  ±5.69%
util/inspect.jsoption='none' method='Object_deep_ln' n=20000                         0.49 %       ±3.02%  ±4.02%  ±5.24%
util/inspect.jsoption='none' method='Object_empty' n=20000                          -3.47 %       ±4.42%  ±5.89%  ±7.67%
util/inspect.jsoption='none' method='Object' n=20000                                -1.18 %       ±4.69%  ±6.24%  ±8.14%
util/inspect.jsoption='none' method='Set' n=20000                                   -1.77 %       ±5.03%  ±6.70%  ±8.74%
util/inspect.jsoption='none' method='String_boxed' n=20000                           0.88 %       ±4.49%  ±5.97%  ±7.79%
util/inspect.jsoption='none' method='String_complex' n=20000                         2.98 %       ±6.03%  ±8.03% ±10.46%
util/inspect.jsoption='none' method='String' n=20000                                -1.96 %       ±4.93%  ±6.56%  ±8.55%
util/inspect.jsoption='none' method='TypedArray_extra' n=20000                       1.19 %       ±4.16%  ±5.55%  ±7.26%
util/inspect.jsoption='none' method='TypedArray' n=20000                             1.42 %       ±5.57%  ±7.48%  ±9.88%
util/inspect.jsoption='showHidden' method='Array' n=20000                            1.71 %       ±3.21%  ±4.27%  ±5.57%
util/inspect.jsoption='showHidden' method='Date' n=20000                            -0.01 %       ±5.13%  ±6.83%  ±8.89%
util/inspect.jsoption='showHidden' method='Error' n=20000                           -1.08 %       ±3.46%  ±4.61%  ±6.01%
util/inspect.jsoption='showHidden' method='Number' n=20000                          -1.53 %       ±2.94%  ±3.93%  ±5.15%
util/inspect.jsoption='showHidden' method='Object_deep_ln' n=20000           **      3.80 %       ±2.77%  ±3.69%  ±4.80%
util/inspect.jsoption='showHidden' method='Object_empty' n=20000                     4.11 %       ±4.60%  ±6.15%  ±8.06%
util/inspect.jsoption='showHidden' method='Object' n=20000                           2.09 %       ±3.50%  ±4.67%  ±6.10%
util/inspect.jsoption='showHidden' method='Set' n=20000                              0.22 %       ±4.70%  ±6.26%  ±8.14%
util/inspect.jsoption='showHidden' method='String_boxed' n=20000                     2.63 %       ±5.11%  ±6.81%  ±8.86%
util/inspect.jsoption='showHidden' method='String_complex' n=20000                   1.39 %       ±4.66%  ±6.21%  ±8.08%
util/inspect.jsoption='showHidden' method='String' n=20000                          -3.26 %       ±3.69%  ±4.93%  ±6.45%
util/inspect.jsoption='showHidden' method='TypedArray_extra' n=20000                 1.94 %       ±2.47%  ±3.28%  ±4.27%
util/inspect.jsoption='showHidden' method='TypedArray' n=20000                      -0.12 %       ±1.94%  ±2.58%  ±3.37%
util/inspect-proxy.jsisProxy=0 showProxy=0 n=100000                                  1.85 %       ±2.09%  ±2.78%  ±3.63%
util/inspect-proxy.jsisProxy=0 showProxy=1 n=100000                                 -0.31 %       ±2.06%  ±2.74%  ±3.56%
util/inspect-proxy.jsisProxy=1 showProxy=0 n=100000                                  0.09 %       ±1.45%  ±1.93%  ±2.51%
util/inspect-proxy.jsisProxy=1 showProxy=1 n=100000                                  0.35 %       ±2.27%  ±3.03%  ±3.94%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 51 comparisons, you can thus
expect the following amount of false-positive results:
  2.55 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.51 false positives, when considering a   1% risk acceptance (**, ***),
  0.05 false positives, when considering a 0.1% risk acceptance (***)

[aduh95]

Landed in 029d1fd...742342f

[aduh95]

I tried to run some benchmark for the browser, and it seems a Reflect.apply approach would be much more efficient than the current Function.prototype.apply solution across all browsers: https://jsben.ch/rsXK9

[ExE-Boss]

@aduh95 That’s not what this does:

globalThis.myArrayOf = bind.bind(Reflect.apply, Array.of, null);

That creates a function that performs Reflect.apply.bind(Array.of, null), which is incorrect.

---

A correct implementation is:

globalThis.myArrayOf = call.bind(Reflect.apply, null, Array.of, Array);

or even:

globalThis.myArrayOf = (args) => Reflect.apply(Array.of, Array, args);

Benchmark: https://jsben.ch/jznX9

---

This one shows that ArrayPrototypeSlice is the best at copying arrays than ArrayOfApply and the bound %Function.prototype.apply% is faster than %Reflect.apply%, with the arrow function and bound %Function.prototype.call% being more or less identical.

[aduh95]

@ExE-Boss You're correct, I also tried with Reflect.apply.bind(null, Array.of, null) and Reflect.apply.bind(null, Array.of, Array) and didn't get much more perf improvements.