stream: refactor to use more primordials

[aduh95]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[mscdex]

These changes result in performance regressions:

 confidence improvement accuracy (*) (**) (***) streams/creation.js kind='duplex' n=50000000 *** -20.52 % ±1.74% ±2.32% ±3.03% streams/creation.js kind='readable' n=50000000 *** -2.90 % ±1.60% ±2.14% ±2.79% streams/creation.js kind='transform' n=50000000 *** -40.53 % ±2.19% ±2.94% ±3.87% streams/creation.js kind='writable' n=50000000 *** -17.87 % ±2.23% ±2.97% ±3.87% streams/readable-bigunevenread.js n=1000 *** 36.67 % ±9.91% ±13.19% ±17.18% streams/readable-unevenread.js n=1000 *** -53.23 % ±1.50% ±2.00% ±2.60% streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=2000000 *** -8.27 % ±0.93% ±1.24% ±1.61% streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=2000000 *** -15.26 % ±2.33% ±3.13% ±4.12% streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=2000000 *** -5.86 % ±1.13% ±1.51% ±1.97% streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=2000000 *** -11.91 % ±1.70% ±2.27% ±2.98% streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=2000000 *** -3.08 % ±1.55% ±2.07% ±2.69% streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=2000000 *** -5.16 % ±2.19% ±2.92% ±3.82% 

[mscdex]

Perhaps we should stop using ReflectApply() by default with these refactor PRs as that seems to be one of the major, consistent causes of these regressions?

[benjamingr]

@mcollina does this introduce concerns for readable-stream?

[mcollina]

LGTM, I'll deal with it somehow in readable-stream. This ship has sailed long ago.

(Note that using so many primordials will cause problems in jest).

[mcollina]

Actuall no, this should not land given the perf regressions. Good spot @mscdex!

[aduh95]

@mscdex@mcollina I've spotted what changes where causing the perf regressions and I've revert them. I think this is ready to land now PTAL.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/808/

Benchmark results

 confidence improvement accuracy (*) (**) (***) streams/creation.js kind='duplex' n=50000000 2.34 % ±3.45% ±4.60% ±5.99% streams/creation.js kind='readable' n=50000000 0.50 % ±1.90% ±2.53% ±3.29% streams/creation.js kind='transform' n=50000000 * 3.77 % ±3.20% ±4.26% ±5.54% streams/creation.js kind='writable' n=50000000 ** 4.19 % ±2.95% ±3.94% ±5.16% streams/pipe.js n=5000000 -1.44 % ±3.70% ±4.95% ±6.51% streams/pipe-object-mode.js n=5000000 -0.47 % ±2.14% ±2.85% ±3.72% streams/readable-async-iterator.js sync='no' n=100000 0.27 % ±2.42% ±3.22% ±4.19% streams/readable-async-iterator.js sync='yes' n=100000 1.94 % ±3.37% ±4.49% ±5.84% streams/readable-bigread.js n=1000 0.18 % ±3.81% ±5.08% ±6.63% streams/readable-bigunevenread.js n=1000 3.98 % ±6.60% ±8.83% ±11.59% streams/readable-boundaryread.js type='buffer' n=2000 0.52 % ±3.20% ±4.25% ±5.53% streams/readable-boundaryread.js type='string' n=2000 0.02 % ±1.27% ±1.68% ±2.19% streams/readable-readall.js n=5000 ** 3.88 % ±2.67% ±3.56% ±4.63% streams/readable-unevenread.js n=1000 1.17 % ±2.49% ±3.31% ±4.32% streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=2000000 0.07 % ±1.02% ±1.35% ±1.76% streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=2000000 -2.41 % ±2.94% ±3.91% ±5.09% streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=2000000 -1.45 % ±2.74% ±3.65% ±4.75% streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=2000000 2.45 % ±2.76% ±3.69% ±4.81% streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=2000000 -0.21 % ±1.60% ±2.13% ±2.78% streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=2000000 -0.54 % ±3.72% ±4.95% ±6.47% streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=2000000 1.62 % ±2.15% ±2.87% ±3.74% streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=2000000 0.23 % ±3.39% ±4.52% ±5.88% streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=2000000 1.94 % ±2.61% ±3.50% ±4.59% streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=2000000 -0.09 % ±1.37% ±1.83% ±2.40% streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=2000000 * -2.64 % ±2.09% ±2.79% ±3.64% streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=2000000 * -3.05 % ±2.67% ±3.55% ±4.64% streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=2000000 0.04 % ±2.01% ±2.68% ±3.50% streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=2000000 0.27 % ±1.26% ±1.69% ±2.21% streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=2000000 1.57 % ±2.86% ±3.82% ±5.00% streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=2000000 1.12 % ±1.94% ±2.58% ±3.36% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case there are 30 comparisons, you can thus expect the following amount of false-positive results: 1.50 false positives, when considering a 5% risk acceptance (*, **, ***), 0.30 false positives, when considering a 1% risk acceptance (**, ***), 0.03 false positives, when considering a 0.1% risk acceptance (***) 

[nodejs-github-bot]

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

[aduh95]

Another benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/847/

 streams/creation.js kind='writable' n=50000000 *** 7.11 % ±2.60% ±3.46% ±4.51% streams/readable-boundaryread.js type='string' n=2000 ** 2.31 % ±1.43% ±1.91% ±2.51% 

Details

 confidence improvement accuracy (*) (**) (***) streams/creation.js kind='duplex' n=50000000 -0.03 % ±2.11% ±2.81% ±3.66% streams/creation.js kind='readable' n=50000000 -0.31 % ±1.76% ±2.34% ±3.04% streams/creation.js kind='transform' n=50000000 -0.18 % ±2.42% ±3.22% ±4.20% streams/creation.js kind='writable' n=50000000 *** 7.11 % ±2.60% ±3.46% ±4.51% streams/pipe.js n=5000000 -0.93 % ±2.37% ±3.16% ±4.12% streams/pipe-object-mode.js n=5000000 0.63 % ±2.52% ±3.35% ±4.36% streams/readable-async-iterator.js sync='no' n=100000 -1.24 % ±1.95% ±2.60% ±3.38% streams/readable-async-iterator.js sync='yes' n=100000 0.80 % ±3.56% ±4.74% ±6.17% streams/readable-bigread.js n=1000 -1.74 % ±3.00% ±3.99% ±5.20% streams/readable-bigunevenread.js n=1000 4.02 % ±6.83% ±9.15% ±12.06% streams/readable-boundaryread.js type='buffer' n=2000 -0.43 % ±3.91% ±5.20% ±6.78% streams/readable-boundaryread.js type='string' n=2000 ** 2.31 % ±1.43% ±1.91% ±2.51% streams/readable-readall.js n=5000 -0.34 % ±1.97% ±2.63% ±3.42% streams/readable-unevenread.js n=1000 -0.12 % ±1.90% ±2.52% ±3.29% streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=2000000 -0.10 % ±0.89% ±1.18% ±1.54% streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=2000000 2.23 % ±3.20% ±4.26% ±5.54% streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=2000000 1.30 % ±2.20% ±2.93% ±3.83% streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=2000000 -2.35 % ±2.76% ±3.68% ±4.79% streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=2000000 0.76 % ±2.97% ±3.97% ±5.21% streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=2000000 -1.98 % ±4.34% ±5.78% ±7.53% streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=2000000 -0.78 % ±2.27% ±3.03% ±3.97% streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=2000000 1.11 % ±3.34% ±4.45% ±5.79% streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=2000000 -0.01 % ±2.01% ±2.67% ±3.48% streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=2000000 1.00 % ±1.74% ±2.32% ±3.05% streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=2000000 1.12 % ±3.05% ±4.06% ±5.28% streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=2000000 1.80 % ±2.89% ±3.85% ±5.03% streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=2000000 0.45 % ±2.16% ±2.87% ±3.74% streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=2000000 -0.57 % ±1.68% ±2.24% ±2.92% streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=2000000 0.12 % ±1.77% ±2.35% ±3.06% streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=2000000 -1.14 % ±1.67% ±2.22% ±2.89% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case there are 30 comparisons, you can thus expect the following amount of false-positive results: 1.50 false positives, when considering a 5% risk acceptance (*, **, ***), 0.30 false positives, when considering a 1% risk acceptance (**, ***), 0.03 false positives, when considering a 0.1% risk acceptance (***) 

[aduh95]

ping @mcollina@mscdex

Dismissing for unresponsiveness.

[aduh95]

@mcollina@mscdex I've dismissed the change requests as I believe I've addressed your objections already and my pings haven't received any response in more than seven days. If you want to have another look to give this PR a green mark or want to clarify you are still objecting, you're of course welcome to do so :)

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[RaisinTen]

node/lib/internal/streams/buffer_list.js

Lines 82 to 83 in c746c40

	constslice=data.slice(0,n);
	this.head.data=data.slice(n);

Should we use StringPrototypeSlice here too?

[aduh95]

node/lib/internal/streams/buffer_list.js

Lines 82 to 83 in c746c40

	constslice=data.slice(0,n);
	this.head.data=data.slice(n);

Should we use StringPrototypeSlice here too?

That's what I did originally and I reverted it in b93fda8 for performance reason.

[mcollina]

Bench CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/925/

[RaisinTen]

Benchmark (hope this one doesn't hang): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/924/
@aduh95 benchmark results look good, cancelled it. 👍

[aduh95]

I've already ran a benchmark CI for #37126 which does include the changes in this PR and got acceptable results, see #37126 (comment). I don't think it's necessary to run another benchmark, and I'd be happy to skip it if people in this thread agree.

[nodejs-github-bot]

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

[benjamingr]

Not blocking - just pointing out that the reason this is taking you long to land is that the streams people (me included) don't feel as strongly about the primordial stuff as they do about not breaking the ecosystem (since everything builds on streams) and this PR has breakage potential.

Just writing it down so you don't think you are being ignored - these sort of changes are just very scary (in terms of potential breakage) and somewhat complicate the code.

❤️

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

Landed in 419686c

@benjamingr Thanks for your message! I understand the concerns, and I'm willing to work to help solve any breakage that might arise from these changes.

[mcollina]

I don't think this should have been landed without a @nodejs/streams approval.
Moreover, the benchmark CI I started was interrupted after I wanted to check the changes from this PR.

We must check if this causes any regressions and consider a revert. I've added a bunch of dont-land tags so we can investigate before backporting.

[mcollina]

What do you think @nodejs/tsc?

[mcollina]

CITGM of master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2606/

[aduh95]

I've opened a revert PR here #37168, we can run benchmarks on top of it to test the impact of this PR. FWIW I've run several benchmarks myself to remove all changes that was causing perf regressions and I've removed those in b93fda8. There is also a recent benchmark CI that was run and didn't show any significant perf regression (https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/913/).

[benjamingr]

I'm confused, did benchmarks not run after the last changes here?

Edit: I think they did run "on top" of it in https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/913/