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 avoid unsafe array iteration #37126

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 29, 2021

This PR includes changes from #36346 (which has not landed yet pending reviews), please review only 1666ca7 on this thread.

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 29, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 29, 2021

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

EDIT: No significant regressions, and one perf improvement:

streams/creation.jskind='writable' n=50000000           ***      6.33 %       ±2.30% ±3.07% ±4.02%
                                                                                         confidence improvement accuracy (*)   (**)  (***)
streams/creation.jskind='duplex' n=50000000                                                             -0.91 %       ±4.65% ±6.19% ±8.06%
streams/creation.jskind='readable' n=50000000                                                            0.23 %       ±2.05% ±2.74% ±3.57%
streams/creation.jskind='transform' n=50000000                                                           0.29 %       ±3.01% ±4.00% ±5.21%
streams/creation.jskind='writable' n=50000000                                                   ***      6.33 %       ±2.30% ±3.07% ±4.02%
streams/pipe.jsn=5000000                                                                                -0.14 %       ±2.60% ±3.47% ±4.54%
streams/pipe-object-mode.jsn=5000000                                                                    -1.80 %       ±2.37% ±3.17% ±4.16%
streams/readable-async-iterator.jssync='no' n=100000                                                    -1.58 %       ±2.10% ±2.79% ±3.64%
streams/readable-async-iterator.jssync='yes' n=100000                                                   -2.15 %       ±3.86% ±5.14% ±6.70%
streams/readable-bigread.jsn=1000                                                                       -0.40 %       ±3.14% ±4.18% ±5.45%
streams/readable-bigunevenread.jsn=1000                                                                 -0.60 %       ±3.73% ±4.98% ±6.49%
streams/readable-boundaryread.jstype='buffer' n=2000                                                    -0.72 %       ±2.16% ±2.87% ±3.74%
streams/readable-boundaryread.jstype='string' n=2000                                                     0.21 %       ±1.47% ±1.96% ±2.55%
streams/readable-readall.jsn=5000                                                                        0.54 %       ±3.80% ±5.05% ±6.58%
streams/readable-unevenread.jsn=1000                                                                     0.44 %       ±3.08% ±4.10% ±5.35%
streams/writable-manywrites.jslen=1024 callback='no' writev='no' sync='no' n=2000000                    -0.01 %       ±1.94% ±2.60% ±3.42%
streams/writable-manywrites.jslen=1024 callback='no' writev='no' sync='yes' n=2000000                   -2.56 %       ±3.36% ±4.47% ±5.83%
streams/writable-manywrites.jslen=1024 callback='no' writev='yes' sync='no' n=2000000                   -0.28 %       ±2.66% ±3.54% ±4.61%
streams/writable-manywrites.jslen=1024 callback='no' writev='yes' sync='yes' n=2000000            *     -3.57 %       ±2.91% ±3.87% ±5.05%
streams/writable-manywrites.jslen=1024 callback='yes' writev='no' sync='no' n=2000000                    0.29 %       ±1.14% ±1.52% ±1.98%
streams/writable-manywrites.jslen=1024 callback='yes' writev='no' sync='yes' n=2000000                  -0.09 %       ±3.34% ±4.45% ±5.79%
streams/writable-manywrites.jslen=1024 callback='yes' writev='yes' sync='no' n=2000000                   1.18 %       ±2.32% ±3.09% ±4.04%
streams/writable-manywrites.jslen=1024 callback='yes' writev='yes' sync='yes' n=2000000                  0.46 %       ±2.77% ±3.68% ±4.79%
streams/writable-manywrites.jslen=32768 callback='no' writev='no' sync='no' n=2000000                    0.63 %       ±2.33% ±3.12% ±4.11%
streams/writable-manywrites.jslen=32768 callback='no' writev='no' sync='yes' n=2000000            *      1.21 %       ±1.08% ±1.44% ±1.87%
streams/writable-manywrites.jslen=32768 callback='no' writev='yes' sync='no' n=2000000                  -0.50 %       ±1.47% ±1.96% ±2.55%
streams/writable-manywrites.jslen=32768 callback='no' writev='yes' sync='yes' n=2000000                 -0.25 %       ±1.73% ±2.31% ±3.00%
streams/writable-manywrites.jslen=32768 callback='yes' writev='no' sync='no' n=2000000                  -1.15 %       ±2.46% ±3.30% ±4.35%
streams/writable-manywrites.jslen=32768 callback='yes' writev='no' sync='yes' n=2000000                 -0.21 %       ±1.38% ±1.84% ±2.39%
streams/writable-manywrites.jslen=32768 callback='yes' writev='yes' sync='no' n=2000000                 -1.13 %       ±1.82% ±2.42% ±3.16%
streams/writable-manywrites.jslen=32768 callback='yes' writev='yes' sync='yes' n=2000000                 0.62 %       ±1.79% ±2.39% ±3.12%

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

@Lxxyx Lxxyx added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 30, 2021
@aduh95 aduh95 added the blocked PRs that are blocked by other issues or PRs. label Feb 1, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 1, 2021

This is blocked by #36346. Can I get a green tick over there as I think it's ready to land? (maybe from @Lxxyx?)

PR-URL: nodejs#37126
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@aduh95 aduh95 removed the blocked PRs that are blocked by other issues or PRs. label Feb 1, 2021
@aduh95 aduh95 merged commit 4ad46e2 into nodejs:master Feb 1, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 1, 2021

Landed in 4ad46e2

@aduh95 aduh95 deleted the stream-array-iteration branch February 1, 2021 14:23
@mcollina
Copy link
Member

mcollina commented Feb 1, 2021

I'm a bit concerned that so many changes to streams lands without a @nodejs/streams approval.

@nodejs/tsc wdyt?

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

6 participants