stream: refactor to use more primordials by aduh95 · Pull Request #36346 · nodejs/node · GitHub

Checklist

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

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%

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?

@mcollina does this introduce concerns for readable-stream?

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

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

@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 (***)

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

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 (***)

ping @mcollina @mscdex

Dismissing for unresponsiveness.

@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 :)

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

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

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?

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.

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

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

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.

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

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.

❤️

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

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>

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.

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.

What do you think @nodejs/tsc?

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

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/).

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

Comment thread lib/internal/streams/writable.js Outdated

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 outstanding review comments, and a CI started. and removed review wanted PRs that need reviews. labels Feb 1, 2021

RaisinTen reviewed Feb 1, 2021

View reviewed changes

Comment thread lib/internal/streams/buffer_list.js Outdated

RaisinTen reviewed Feb 1, 2021

View reviewed changes

Comment thread lib/internal/streams/writable.js Outdated

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

stream: refactor to use more primordials by aduh95 · Pull Request #36346 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: refactor to use more primordials - #36346

stream: refactor to use more primordials#36346aduh95 merged 1 commit intonodejs:masternodejs/node:masterfrom aduh95:streams-primordialsaduh95/node:streams-primordialsCopy head branch name to clipboard

Conversation

aduh95 commented Dec 1, 2020

Checklist

Uh oh!

mscdex left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Dec 2, 2020

Uh oh!

benjamingr commented Dec 2, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

aduh95 commented Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 27, 2020

Uh oh!

aduh95 commented Jan 2, 2021

Uh oh!

aduh95 commented Jan 5, 2021

Uh oh!

aduh95 commented Jan 11, 2021

Uh oh!

nodejs-github-bot commented Feb 1, 2021

Uh oh!

nodejs-github-bot commented Feb 1, 2021

Uh oh!

Uh oh!

RaisinTen commented Feb 1, 2021

Uh oh!

aduh95 commented Feb 1, 2021

Uh oh!

Uh oh!

mcollina commented Feb 1, 2021

Uh oh!

RaisinTen commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Feb 1, 2021

Uh oh!

nodejs-github-bot commented Feb 1, 2021

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 1, 2021

Uh oh!

nodejs-github-bot commented Feb 1, 2021

Uh oh!

aduh95 commented Feb 1, 2021

Uh oh!

mcollina commented Feb 1, 2021

Uh oh!

mcollina commented Feb 1, 2021

Uh oh!

mcollina commented Feb 1, 2021

Uh oh!

aduh95 commented Feb 1, 2021

Uh oh!

benjamingr commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

stream: refactor to use more primordials#36346
aduh95 merged 1 commit into
nodejs:masterfrom
aduh95:streams-primordials

aduh95 commented Dec 27, 2020 •
edited
Loading

RaisinTen commented Feb 1, 2021 •
edited
Loading

benjamingr commented Feb 1, 2021 •
edited
Loading