Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: refactor to use more primordials #36346

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 1, 2020

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

@nodejs-github-bot 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
mscdex previously requested changes Dec 2, 2020
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

mscdex commented Dec 2, 2020

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
Copy link
Member

@mcollina does this introduce concerns for readable-stream?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aduh95

This comment has been minimized.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 27, 2020

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

@aduh95 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020
@nodejs-github-bot
Copy link
Collaborator

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

aduh95 commented Jan 2, 2021

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
Copy link
Contributor Author

aduh95 commented Jan 5, 2021

ping @mcollina @mscdex

@aduh95 aduh95 dismissed stale reviews from mcollina and mscdex January 11, 2021 10:13

Dismissing for unresponsiveness.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 11, 2021

@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
Copy link
Collaborator

@Lxxyx 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
@nodejs-github-bot
Copy link
Collaborator

@RaisinTen
Copy link
Contributor

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

Should we use StringPrototypeSlice here too?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 1, 2021

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.

@mcollina
Copy link
Member

mcollina commented Feb 1, 2021

@RaisinTen
Copy link
Contributor

RaisinTen commented Feb 1, 2021

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
Copy link
Contributor Author

aduh95 commented Feb 1, 2021

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
Copy link
Collaborator

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

PR-URL: nodejs#36346
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@aduh95 aduh95 merged commit 419686c into nodejs:master Feb 1, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 1, 2021

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.

@aduh95 aduh95 deleted the streams-primordials branch February 1, 2021 14:20
@mcollina
Copy link
Member

mcollina commented Feb 1, 2021

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 mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 1, 2021
@mcollina
Copy link
Member

mcollina commented Feb 1, 2021

What do you think @nodejs/tsc?

@mcollina
Copy link
Member

mcollina commented Feb 1, 2021

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

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 1, 2021

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
Copy link
Member

benjamingr commented Feb 1, 2021

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/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants