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

Revert stream primordials #37168

Closed
wants to merge 2 commits into from
Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 1, 2021

Revert PR to run benchmarks.

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 1, 2021
@aduh95 aduh95 requested a review from mcollina February 1, 2021 14:41
@mcollina
Copy link
Member

mcollina commented Feb 1, 2021

Let's just run the benchmark CI on this. We can close if not needed. Thanks

@mcollina
Copy link
Member

mcollina commented Feb 1, 2021

The perf gain is there for Writable (minus is good):

17:26:39 streams/creation.jskind='duplex' n=50000000                                                             -0.31 %       ±2.33%  ±3.10%  ±4.04%
17:26:39 streams/creation.jskind='readable' n=50000000                                                            0.50 %       ±1.69%  ±2.24%  ±2.92%
17:26:39 streams/creation.jskind='transform' n=50000000                                                           0.90 %       ±3.56%  ±4.75%  ±6.19%
17:26:39 streams/creation.jskind='writable' n=50000000                                                   ***     -5.69 %       ±2.12%  ±2.82%  ±3.68%
17:26:39 streams/pipe.jsn=5000000                                                                                 1.43 %       ±2.77%  ±3.68%  ±4.79%
17:26:39 streams/pipe-object-mode.jsn=5000000                                                                    -1.45 %       ±3.54%  ±4.71%  ±6.12%
17:26:39 streams/readable-async-iterator.jssync='no' n=100000                                                     1.44 %       ±2.85%  ±3.79%  ±4.94%
17:26:39 streams/readable-async-iterator.jssync='yes' n=100000                                                    2.84 %       ±5.29%  ±7.03%  ±9.16%
17:26:39 streams/readable-bigread.jsn=1000                                                                       -3.32 %       ±4.55%  ±6.06%  ±7.90%
17:26:39 streams/readable-bigunevenread.jsn=1000                                                                 -0.55 %       ±7.71% ±10.26% ±13.36%
17:26:39 streams/readable-boundaryread.jstype='buffer' n=2000                                                     1.74 %       ±3.59%  ±4.81%  ±6.32%
17:26:39 streams/readable-boundaryread.jstype='string' n=2000                                                     1.67 %       ±1.82%  ±2.44%  ±3.23%
17:26:39 streams/readable-readall.jsn=5000                                                                        0.81 %       ±2.56%  ±3.40%  ±4.43%
17:26:39 streams/readable-unevenread.jsn=1000                                                                    -1.52 %       ±2.89%  ±3.85%  ±5.01%
17:26:39 streams/writable-manywrites.jslen=1024 callback='no' writev='no' sync='no' n=2000000                    -1.17 %       ±1.33%  ±1.77%  ±2.31%
17:26:39 streams/writable-manywrites.jslen=1024 callback='no' writev='no' sync='yes' n=2000000                   -1.10 %       ±3.89%  ±5.18%  ±6.74%
17:26:39 streams/writable-manywrites.jslen=1024 callback='no' writev='yes' sync='no' n=2000000                   -1.35 %       ±2.41%  ±3.21%  ±4.18%
17:26:39 streams/writable-manywrites.jslen=1024 callback='no' writev='yes' sync='yes' n=2000000                  -1.07 %       ±3.32%  ±4.42%  ±5.75%
17:26:39 streams/writable-manywrites.jslen=1024 callback='yes' writev='no' sync='no' n=2000000                   -0.54 %       ±1.04%  ±1.38%  ±1.81%
17:26:39 streams/writable-manywrites.jslen=1024 callback='yes' writev='no' sync='yes' n=2000000                   1.03 %       ±3.30%  ±4.41%  ±5.76%
17:26:39 streams/writable-manywrites.jslen=1024 callback='yes' writev='yes' sync='no' n=2000000                   0.41 %       ±2.56%  ±3.40%  ±4.44%
17:26:39 streams/writable-manywrites.jslen=1024 callback='yes' writev='yes' sync='yes' n=2000000                 -0.84 %       ±3.83%  ±5.10%  ±6.65%
17:26:39 streams/writable-manywrites.jslen=32768 callback='no' writev='no' sync='no' n=2000000                    0.77 %       ±1.49%  ±1.99%  ±2.59%
17:26:39 streams/writable-manywrites.jslen=32768 callback='no' writev='no' sync='yes' n=2000000                  -0.30 %       ±2.33%  ±3.10%  ±4.04%
17:26:39 streams/writable-manywrites.jslen=32768 callback='no' writev='yes' sync='no' n=2000000                  -1.40 %       ±4.64%  ±6.22%  ±8.18%
17:26:39 streams/writable-manywrites.jslen=32768 callback='no' writev='yes' sync='yes' n=2000000                  0.76 %       ±1.60%  ±2.13%  ±2.78%
17:26:39 streams/writable-manywrites.jslen=32768 callback='yes' writev='no' sync='no' n=2000000                   0.52 %       ±1.38%  ±1.85%  ±2.41%
17:26:39 streams/writable-manywrites.jslen=32768 callback='yes' writev='no' sync='yes' n=2000000                 -0.04 %       ±1.68%  ±2.24%  ±2.91%
17:26:39 streams/writable-manywrites.jslen=32768 callback='yes' writev='yes' sync='no' n=2000000                 -1.21 %       ±1.55%  ±2.06%  ±2.69%
17:26:39 streams/writable-manywrites.jslen=32768 callback='yes' writev='yes' sync='yes' n=2000000                -0.91 %       ±2.08%  ±2.77%  ±3.60%

@mcollina
Copy link
Member

mcollina commented Feb 1, 2021

I want to wait on http as well.

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 2, 2021

@mcollina It seems the CI is hanging (it's been 16 hours since a log was emitted to the console output). It might be worth splitting it into several jobs.

@mcollina
Copy link
Member

mcollina commented Feb 2, 2021

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 2, 2021

CI is stuck again (last log was 1 hour ago)

@mcollina
Copy link
Member

mcollina commented Feb 5, 2021

52 hours later, the benches are still running on my server. Anyway, the preliminary results are good (no regression) closing this.

@mcollina mcollina closed this Feb 5, 2021
@aduh95 aduh95 deleted the revert-stream-primordials branch February 5, 2021 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants