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!

lib/internal/streams/writable.js

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: nodejs#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 Issues and PRs related to general changes in the lib or src directory. label Dec 1, 2020

mscdex previously requested changes Dec 2, 2020

View reviewed changes

mcollina approved these changes Dec 2, 2020

View reviewed changes

mcollina previously requested changes Dec 2, 2020

View reviewed changes

ExE-Boss reviewed Dec 2, 2020

View reviewed changes

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

aduh95 force-pushed the streams-primordials branch from 5691ce5 to 1ea7378 Compare December 7, 2020 20:09

This comment has been minimized.

Sign in to view

aduh95 requested review from mcollina and mscdex December 27, 2020 09:19

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. review wanted PRs that need reviews. labels Dec 27, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020

aduh95 force-pushed the streams-primordials branch 2 times, most recently from c96d03a to 8015a72 Compare December 30, 2020 15:28

aduh95 force-pushed the streams-primordials branch from 8015a72 to b93fda8 Compare January 28, 2021 18:56

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 reviewed changes

Lxxyx added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels Feb 1, 2021

RaisinTen reviewed Feb 1, 2021

View reviewed changes

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

RaisinTen reviewed Feb 1, 2021

View reviewed changes

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

RaisinTen approved these changes Feb 1, 2021

View reviewed changes

benjamingr reviewed Feb 1, 2021

View reviewed changes

stream: refactor to use more primordials

419686c

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

aduh95 force-pushed the streams-primordials branch from f72fa25 to 419686c Compare February 1, 2021 14:19

aduh95 merged commit 419686c into nodejs:master Feb 1, 2021

aduh95 deleted the streams-primordials branch February 1, 2021 14:20

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

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 1, 2021

BethGriggs mentioned this pull request Apr 12, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 minimized.

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

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

Choose a reason for hiding this comment

mscdex commented Dec 2, 2020

benjamingr commented Dec 2, 2020

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

This comment has been minimized.

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

Choose a reason for hiding this 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