lib: replace spread operator with primordials function #54053

MCprotein · 2024-07-26T09:06:12Z

replaced the spread operator with ArrayPrototypeSlice to avoid reliance on user-mutable methods and enhance the safety of array iteration

Refs:
https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md#unsafe-array-iteration

nodejs-github-bot · 2024-07-26T09:06:17Z

Review requested:

@nodejs/streams

mcollina

lgtm

MCprotein · 2024-07-26T12:51:53Z

lgtm

thank you so much

replaced the spread operator with ArrayPrototypeSlice to avoid reliance on user-mutable methods and enhance the safety of array iteration Refs: https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md#unsafe-array-iteration

MCprotein · 2024-07-26T13:03:48Z

fixed js lint

anonrig

We need benchmarks to ensure this doesn't cause a regression.

MCprotein · 2024-07-26T14:30:07Z

We need benchmarks to ensure this doesn't cause a regression.

@anonrig Are you referring to https://github.com/nodejs/node/blob/main/doc/contributing/writing-and-running-benchmarks.md?

MCprotein · 2024-07-28T12:19:20Z

We need benchmarks to ensure this doesn't cause a regression.

@anonrig
Here are the benchmark results.
Please check them.

streams/creation.js kind='duplex' n=50000000                                                    ***     67.57 %       ±2.58% ±3.43% ±4.47%
streams/creation.js kind='readable' n=50000000                                                          -0.58 %       ±1.39% ±1.86% ±2.42%
streams/creation.js kind='transform' n=50000000                                                         -0.85 %       ±1.48% ±1.97% ±2.57%
streams/creation.js kind='writable' n=50000000                                                    *     -2.01 %       ±2.01% ±2.69% ±3.53%
streams/destroy.js kind='duplex' n=1000000                                                      ***      6.89 %       ±2.29% ±3.05% ±3.97%
streams/destroy.js kind='readable' n=1000000                                                            -0.12 %       ±2.57% ±3.43% ±4.47%
streams/destroy.js kind='transform' n=1000000                                                           -0.64 %       ±1.96% ±2.60% ±3.39%
streams/destroy.js kind='writable' n=1000000                                                             1.30 %       ±2.30% ±3.06% ±3.99%
streams/pipe-object-mode.js n=5000000                                                                    1.47 %       ±1.82% ±2.44% ±3.23%
streams/pipe.js n=5000000                                                                               -0.15 %       ±0.90% ±1.21% ±1.59%
streams/readable-async-iterator.js sync='no' n=100000                                                    0.06 %       ±0.95% ±1.27% ±1.65%
streams/readable-async-iterator.js sync='yes' n=100000                                                   0.37 %       ±1.41% ±1.87% ±2.44%
streams/readable-bigread.js n=1000                                                                      -0.15 %       ±1.33% ±1.77% ±2.30%
streams/readable-bigunevenread.js n=1000                                                                 0.10 %       ±0.72% ±0.96% ±1.25%
streams/readable-boundaryread.js type='buffer' n=2000                                                   -0.86 %       ±1.06% ±1.41% ±1.84%
streams/readable-boundaryread.js type='string' n=2000                                                   -0.13 %       ±0.75% ±1.00% ±1.31%
streams/readable-from.js type='array' n=10000000                                                        -1.32 %       ±4.02% ±5.37% ±7.01%
streams/readable-from.js type='async-generator' n=10000000                                        *      1.16 %       ±0.99% ±1.32% ±1.73%
streams/readable-from.js type='sync-generator-with-async-values' n=10000000                             -0.21 %       ±1.40% ±1.86% ±2.43%
streams/readable-from.js type='sync-generator-with-sync-values' n=10000000                              -0.18 %       ±0.49% ±0.66% ±0.86%
streams/readable-readall.js n=5000                                                                       1.04 %       ±2.06% ±2.77% ±3.67%
streams/readable-uint8array.js kind='encoding' n=1000000                                         **     -2.76 %       ±1.99% ±2.66% ±3.49%
streams/readable-uint8array.js kind='read' n=1000000                                                     0.35 %       ±2.77% ±3.69% ±4.80%
streams/readable-unevenread.js n=1000                                                                    0.07 %       ±0.49% ±0.65% ±0.84%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=100000                    -0.12 %       ±0.86% ±1.15% ±1.49%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=100000           ***      2.30 %       ±1.24% ±1.66% ±2.17%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=100000                    0.77 %       ±1.17% ±1.56% ±2.03%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=100000           **      1.86 %       ±1.32% ±1.76% ±2.30%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=100000                   -0.35 %       ±1.08% ±1.44% ±1.88%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=100000           **      1.39 %       ±0.95% ±1.27% ±1.66%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=100000                   0.71 %       ±1.25% ±1.66% ±2.16%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=100000                 -1.02 %       ±1.20% ±1.60% ±2.09%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=100000                   -0.59 %       ±1.43% ±1.90% ±2.47%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=100000                   0.93 %       ±1.30% ±1.74% ±2.26%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=100000          ***      1.43 %       ±0.69% ±0.92% ±1.20%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=100000                 -0.39 %       ±1.37% ±1.82% ±2.37%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=100000                  -0.10 %       ±1.91% ±2.55% ±3.35%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=100000                  0.48 %       ±0.79% ±1.05% ±1.38%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=100000                 -0.17 %       ±0.87% ±1.15% ±1.50%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=100000                 0.46 %       ±1.03% ±1.37% ±1.79%
streams/writable-uint8array.js kind='object-mode' n=50000000                                            -0.32 %       ±0.49% ±0.66% ±0.86%
streams/writable-uint8array.js kind='write' n=50000000                                                  -0.64 %       ±0.90% ±1.20% ±1.58%
streams/writable-uint8array.js kind='writev' n=50000000                                                 -0.26 %       ±0.76% ±1.01% ±1.32%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 43 comparisons, you can thus expect the following amount of false-positive results:
  2.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.43 false positives, when considering a   1% risk acceptance (**, ***),
  0.04 false positives, when considering a 0.1% risk acceptance (***)

jakecastelli · 2024-07-28T14:55:07Z

Benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1581/

MCprotein · 2024-07-29T02:05:11Z

Benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1581/

CI is completed and the following results have been produced. Could you check it?

                                                                                         confidence improvement accuracy (*)    (**)   (***)
streams/creation.js kind='duplex' n=50000000                                                            -0.12 %       ±0.45%  ±0.59%  ±0.77%
streams/creation.js kind='readable' n=50000000                                                           0.09 %       ±0.57%  ±0.76%  ±0.99%
streams/creation.js kind='transform' n=50000000                                                         -0.02 %       ±1.97%  ±2.62%  ±3.42%
streams/creation.js kind='writable' n=50000000                                                          -0.18 %       ±0.64%  ±0.85%  ±1.11%
streams/destroy.js kind='duplex' n=1000000                                                              -0.41 %       ±0.64%  ±0.85%  ±1.10%
streams/destroy.js kind='readable' n=1000000                                                             0.39 %       ±0.59%  ±0.79%  ±1.03%
streams/destroy.js kind='transform' n=1000000                                                            0.13 %       ±0.46%  ±0.61%  ±0.80%
streams/destroy.js kind='writable' n=1000000                                                             0.29 %       ±0.61%  ±0.81%  ±1.05%
streams/pipe-object-mode.js n=5000000                                                                   -0.00 %       ±0.26%  ±0.34%  ±0.44%
streams/pipe.js n=5000000                                                                                0.00 %       ±0.37%  ±0.49%  ±0.64%
streams/readable-async-iterator.js sync='no' n=100000                                                    0.49 %       ±0.49%  ±0.66%  ±0.85%
streams/readable-async-iterator.js sync='yes' n=100000                                                   0.01 %       ±0.69%  ±0.91%  ±1.19%
streams/readable-bigread.js n=1000                                                                       0.20 %       ±0.50%  ±0.66%  ±0.86%
streams/readable-bigunevenread.js n=1000                                                                 0.39 %       ±0.59%  ±0.79%  ±1.03%
streams/readable-boundaryread.js type='buffer' n=2000                                                    0.30 %       ±0.93%  ±1.23%  ±1.60%
streams/readable-boundaryread.js type='string' n=2000                                                   -0.11 %       ±0.82%  ±1.09%  ±1.42%
streams/readable-from.js type='array' n=10000000                                                         2.21 %       ±8.50% ±11.31% ±14.72%
streams/readable-from.js type='async-generator' n=10000000                                               0.50 %       ±0.68%  ±0.91%  ±1.19%
streams/readable-from.js type='sync-generator-with-async-values' n=10000000                             -0.15 %       ±0.28%  ±0.38%  ±0.49%
streams/readable-from.js type='sync-generator-with-sync-values' n=10000000                              -0.06 %       ±0.21%  ±0.28%  ±0.36%
streams/readable-readall.js n=5000                                                                       0.36 %       ±2.32%  ±3.09%  ±4.03%
streams/readable-uint8array.js kind='encoding' n=1000000                                                 0.15 %       ±0.56%  ±0.74%  ±0.97%
streams/readable-uint8array.js kind='read' n=1000000                                                    -1.32 %       ±1.85%  ±2.46%  ±3.20%
streams/readable-unevenread.js n=1000                                                                   -0.16 %       ±0.57%  ±0.76%  ±1.00%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=100000                     0.06 %       ±0.65%  ±0.86%  ±1.13%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=100000                    0.09 %       ±0.76%  ±1.01%  ±1.32%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=100000                    0.75 %       ±1.34%  ±1.80%  ±2.38%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=100000                  -0.64 %       ±1.05%  ±1.40%  ±1.83%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=100000                   -0.09 %       ±0.84%  ±1.12%  ±1.46%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=100000                  -0.17 %       ±1.23%  ±1.64%  ±2.14%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=100000           **     -0.99 %       ±0.65%  ±0.87%  ±1.13%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=100000                 -0.21 %       ±1.66%  ±2.22%  ±2.91%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=100000                   -0.19 %       ±0.83%  ±1.10%  ±1.43%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=100000                   0.14 %       ±1.00%  ±1.33%  ±1.74%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=100000                   0.32 %       ±0.74%  ±0.98%  ±1.28%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=100000                  0.22 %       ±0.78%  ±1.04%  ±1.36%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=100000                   0.14 %       ±0.90%  ±1.20%  ±1.57%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=100000                  0.73 %       ±0.97%  ±1.29%  ±1.68%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=100000                 -0.28 %       ±0.65%  ±0.87%  ±1.13%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=100000                 0.14 %       ±1.04%  ±1.39%  ±1.81%
streams/writable-uint8array.js kind='object-mode' n=50000000                                             3.27 %       ±3.57%  ±4.80%  ±6.36%
streams/writable-uint8array.js kind='write' n=50000000                                            *     -1.80 %       ±1.39%  ±1.85%  ±2.41%
streams/writable-uint8array.js kind='writev' n=50000000                                                 -0.26 %       ±0.32%  ±0.43%  ±0.56%

jakecastelli · 2024-07-29T02:33:27Z

Hi @MCprotein, sorry I just realised we might not have any benchmark for compose in stream yet, so even though the above benchmark results for streams looks good, it probably does not reflect @anonrig's concern.

@anonrig do you mean to add a new a benchmark here? 👀

mcollina

lgtm

nodejs-github-bot · 2024-07-29T06:12:10Z

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

nodejs-github-bot · 2024-07-29T10:14:22Z

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

MCprotein · 2024-07-29T10:39:58Z

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

failed tests may be flaky

node-test-commit-smartos
CI Reliability 2024-07-29 reliability#933
smartos20-64 - reason: parallel/test-net-write-fully-async-buffer
node-test-commit-linux-containered
CI Reliability 2024-07-06 reliability#912
ubi81_sharedlibs_openssl111fips_x64 - reason: parallel/test-stream-readable-unpipe-resume
node-test-commit-arm-fanned, node-test-binary-armv7l
CI Reliability 2023-12-01 reliability#735
js,nodes=ubuntu2004-armv7l - reason: pummel/test-fs-watch-non-recursive

jakecastelli · 2024-07-30T15:21:10Z

The latest CI https://ci.nodejs.org/job/node-test-pull-request/60715/ is green 👍

@anonrig would you mind taking a look again as you are currently blocking this PR 🙏

MCprotein · 2024-07-31T05:24:47Z

@jakecastelli
Thank you so much for your very kind explanation and guidance. I was really touched.

@anonrig
Please explain if there is anything missing. I will proceed as you advised.

jakecastelli · 2024-08-10T12:34:41Z

I wrote a benchmark for stream.compose in #54308 and tested your patch locally, everything looks good! But since the benchmark was requested, we can wait until the benchmark PR to land and then rerun the benchmark to land this one.

benjamingr · 2024-08-10T17:07:10Z

@mcollina I have bad memory, is this fine in terms of readable-stream on npm?

mcollina

lgtm

yes, this is is ok for readable-stream, we have our own primordial implementation.

nodejs-github-bot · 2024-08-12T07:05:41Z

Landed in d7f3730

MCprotein · 2024-08-12T11:40:34Z

@jakecastelli thank you so much.

replaced the spread operator with ArrayPrototypeSlice to avoid reliance on user-mutable methods and enhance the safety of array iteration Refs: https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md#unsafe-array-iteration PR-URL: #54053 Refs: https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md#unsafe-array-iteration Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Jul 26, 2024

mcollina approved these changes Jul 26, 2024

View reviewed changes

MCprotein force-pushed the fix/stream-compose-primordials branch from 5e72978 to d5eee65 Compare July 26, 2024 13:00

anonrig requested changes Jul 26, 2024

View reviewed changes

mcollina approved these changes Jul 29, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024

rluvaton approved these changes Jul 30, 2024

View reviewed changes

jakecastelli mentioned this pull request Aug 10, 2024

benchmark: add stream.compose benchmark #54308

Merged

anonrig approved these changes Aug 10, 2024

View reviewed changes

mcollina approved these changes Aug 12, 2024

View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024

nodejs-github-bot merged commit d7f3730 into nodejs:main Aug 12, 2024
56 checks passed

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: replace spread operator with primordials function #54053

lib: replace spread operator with primordials function #54053

MCprotein commented Jul 26, 2024

nodejs-github-bot commented Jul 26, 2024

mcollina left a comment

MCprotein commented Jul 26, 2024

MCprotein commented Jul 26, 2024

anonrig left a comment

MCprotein commented Jul 26, 2024

MCprotein commented Jul 28, 2024 •

edited

Loading

jakecastelli commented Jul 28, 2024

MCprotein commented Jul 29, 2024 •

edited

Loading

jakecastelli commented Jul 29, 2024

mcollina left a comment

nodejs-github-bot commented Jul 29, 2024

nodejs-github-bot commented Jul 29, 2024

MCprotein commented Jul 29, 2024 •

edited

Loading

jakecastelli commented Jul 30, 2024

MCprotein commented Jul 31, 2024

jakecastelli commented Aug 10, 2024

benjamingr commented Aug 10, 2024

mcollina left a comment

nodejs-github-bot commented Aug 12, 2024

MCprotein commented Aug 12, 2024

lib: replace spread operator with primordials function #54053

lib: replace spread operator with primordials function #54053

Conversation

MCprotein commented Jul 26, 2024

nodejs-github-bot commented Jul 26, 2024

mcollina left a comment

Choose a reason for hiding this comment

MCprotein commented Jul 26, 2024

MCprotein commented Jul 26, 2024

anonrig left a comment

Choose a reason for hiding this comment

MCprotein commented Jul 26, 2024

MCprotein commented Jul 28, 2024 • edited Loading

jakecastelli commented Jul 28, 2024

MCprotein commented Jul 29, 2024 • edited Loading

jakecastelli commented Jul 29, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 29, 2024

nodejs-github-bot commented Jul 29, 2024

MCprotein commented Jul 29, 2024 • edited Loading

jakecastelli commented Jul 30, 2024

MCprotein commented Jul 31, 2024

jakecastelli commented Aug 10, 2024

benjamingr commented Aug 10, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 12, 2024

MCprotein commented Aug 12, 2024

MCprotein commented Jul 28, 2024 •

edited

Loading

MCprotein commented Jul 29, 2024 •

edited

Loading

MCprotein commented Jul 29, 2024 •

edited

Loading