stream: refactor to use more primordials #36346

aduh95 · 2020-12-01T20:31:14Z

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 · 2020-12-02T08:24:11Z

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 · 2020-12-02T15:40:21Z

@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 · 2020-12-27T09:18:45Z

@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 · 2020-12-27T15:31:59Z

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

aduh95 · 2021-01-02T09:00:44Z

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%

                                                                                           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 · 2021-01-05T14:53:15Z

ping @mcollina @mscdex

Dismissing for unresponsiveness.

aduh95 · 2021-01-11T10:19:55Z

@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 · 2021-02-01T09:40:29Z

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

nodejs-github-bot · 2021-02-01T11:00:56Z

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

lib/internal/streams/buffer_list.js

RaisinTen · 2021-02-01T11:27:34Z

node/lib/internal/streams/buffer_list.js

Lines 82 to 83 in c746c40

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

Should we use StringPrototypeSlice here too?

aduh95 · 2021-02-01T11:29:22Z

node/lib/internal/streams/buffer_list.js

Lines 82 to 83 in c746c40

const slice = 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.

lib/internal/streams/writable.js

mcollina · 2021-02-01T11:57:08Z

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

RaisinTen · 2021-02-01T11:57:21Z

~~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 · 2021-02-01T12:02:01Z

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 · 2021-02-01T12:17:52Z

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 · 2021-02-01T12:52:38Z

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

nodejs-github-bot · 2021-02-01T13:40:04Z

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

PR-URL: #36346 Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

aduh95 · 2021-02-01T14:20:06Z

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 · 2021-02-01T14:24:15Z

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 · 2021-02-01T14:26:19Z

What do you think @nodejs/tsc?

mcollina · 2021-02-01T14:30:18Z

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

aduh95 · 2021-02-01T14:41:22Z

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 · 2021-02-01T17:24:12Z

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/

nodejs-github-bot added the lib / src label Dec 1, 2020

mscdex reviewed Dec 2, 2020

View changes

mcollina approved these changes Dec 2, 2020

View changes

mcollina reviewed Dec 2, 2020

View changes

ExE-Boss reviewed Dec 2, 2020

View changes

lib/internal/streams/writable.js Outdated Show resolved Hide resolved

aduh95 force-pushed the aduh95:streams-primordials branch Dec 7, 2020

This comment has been hidden.

Sign in to view

aduh95 requested review from mcollina and mscdex Dec 27, 2020

aduh95 added request-ci review wanted labels Dec 27, 2020

github-actions bot removed the request-ci label Dec 27, 2020

aduh95 force-pushed the aduh95:streams-primordials branch 2 times, most recently to 8015a72 Dec 29, 2020

aduh95 force-pushed the aduh95:streams-primordials branch from 8015a72 to b93fda8 Jan 28, 2021

aduh95 mentioned this pull request Jan 29, 2021

stream: refactor to avoid unsafe array iteration #37126

Merged

Lxxyx approved these changes Feb 1, 2021

View changes

Lxxyx added author ready and removed review wanted labels Feb 1, 2021

RaisinTen reviewed Feb 1, 2021

View changes

lib/internal/streams/buffer_list.js Outdated Show resolved Hide resolved

RaisinTen reviewed Feb 1, 2021

View changes

lib/internal/streams/writable.js Outdated Show resolved Hide resolved

RaisinTen approved these changes Feb 1, 2021

View changes

benjamingr reviewed Feb 1, 2021

View changes

stream: refactor to use more primordials

419686c

PR-URL: #36346 Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

aduh95 force-pushed the aduh95:streams-primordials branch from f72fa25 to 419686c Feb 1, 2021

aduh95 deleted the aduh95:streams-primordials branch Feb 1, 2021

mcollina added dont-land-on-v10.x dont-land-on-v12.x dont-land-on-v14.x labels Feb 1, 2021

mcollina added the tsc-agenda label Feb 1, 2021

BethGriggs mentioned this pull request Apr 12, 2021

2021-04-20, Version 16.0.0 (Current) #37678

Merged

Jul	AUG	Sep
	28
2020	2021	2022

nodejs / node

stream: refactor to use more primordials #36346

stream: refactor to use more primordials #36346

aduh95 commented Dec 1, 2020

mscdex left a comment

mscdex commented Dec 2, 2020

benjamingr commented Dec 2, 2020

mcollina left a comment

mcollina left a comment

This comment has been hidden.

aduh95 commented Dec 27, 2020 •

edited

nodejs-github-bot commented Dec 27, 2020

aduh95 commented Jan 2, 2021

aduh95 commented Jan 5, 2021

aduh95 commented Jan 11, 2021

nodejs-github-bot commented Feb 1, 2021

nodejs-github-bot commented Feb 1, 2021

RaisinTen commented Feb 1, 2021

aduh95 commented Feb 1, 2021

mcollina commented Feb 1, 2021

RaisinTen commented Feb 1, 2021 •

edited

aduh95 commented Feb 1, 2021

nodejs-github-bot commented Feb 1, 2021

benjamingr left a comment

nodejs-github-bot commented Feb 1, 2021

nodejs-github-bot commented Feb 1, 2021

aduh95 commented Feb 1, 2021

mcollina commented Feb 1, 2021

mcollina commented Feb 1, 2021

mcollina commented Feb 1, 2021

aduh95 commented Feb 1, 2021

benjamingr commented Feb 1, 2021 •

edited

nodejs / node

stream: refactor to use more primordials #36346

stream: refactor to use more primordials #36346

Conversation

aduh95 commented Dec 1, 2020

Checklist

mscdex left a comment

mscdex commented Dec 2, 2020

benjamingr commented Dec 2, 2020

mcollina left a comment

mcollina left a comment

This comment has been hidden.

aduh95 commented Dec 27, 2020 • edited

nodejs-github-bot commented Dec 27, 2020

aduh95 commented Jan 2, 2021

aduh95 commented Jan 5, 2021

aduh95 commented Jan 11, 2021

nodejs-github-bot commented Feb 1, 2021

nodejs-github-bot commented Feb 1, 2021

RaisinTen commented Feb 1, 2021

aduh95 commented Feb 1, 2021

mcollina commented Feb 1, 2021

RaisinTen commented Feb 1, 2021 • edited

aduh95 commented Feb 1, 2021

nodejs-github-bot commented Feb 1, 2021

benjamingr left a comment

nodejs-github-bot commented Feb 1, 2021

nodejs-github-bot commented Feb 1, 2021

aduh95 commented Feb 1, 2021

mcollina commented Feb 1, 2021

mcollina commented Feb 1, 2021

mcollina commented Feb 1, 2021

aduh95 commented Feb 1, 2021

benjamingr commented Feb 1, 2021 • edited

aduh95 commented Dec 27, 2020 •

edited

RaisinTen commented Feb 1, 2021 •

edited

benjamingr commented Feb 1, 2021 •

edited