stream: add fast-path for readable streams

[anonrig]

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

Results

                                                                             confidence improvement accuracy (*)    (**)   (***)
streams/readable-encoding.js n=1000 op='push' len=16384 encoding='latin1'                    1.29 %       ±2.75%  ±3.66%  ±4.77%
streams/readable-encoding.js n=1000 op='push' len=16384 encoding='utf-8'            ***     15.70 %       ±4.21%  ±5.61%  ±7.34%
streams/readable-encoding.js n=1000 op='push' len=256 encoding='latin1'                     -5.85 %       ±7.27%  ±9.68% ±12.60%
streams/readable-encoding.js n=1000 op='push' len=256 encoding='utf-8'                      -0.91 %       ±7.21%  ±9.61% ±12.53%
streams/readable-encoding.js n=1000 op='push' len=512 encoding='latin1'              **     -9.23 %       ±6.72%  ±8.94% ±11.64%
streams/readable-encoding.js n=1000 op='push' len=512 encoding='utf-8'                       3.36 %       ±8.07% ±10.74% ±13.98%
streams/readable-encoding.js n=1000 op='unshift' len=16384 encoding='latin1'                 1.49 %       ±4.27%  ±5.68%  ±7.40%
streams/readable-encoding.js n=1000 op='unshift' len=16384 encoding='utf-8'         ***     15.10 %       ±4.09%  ±5.45%  ±7.11%
streams/readable-encoding.js n=1000 op='unshift' len=256 encoding='latin1'           **     -8.54 %       ±5.66%  ±7.55%  ±9.85%
streams/readable-encoding.js n=1000 op='unshift' len=256 encoding='utf-8'                   -6.27 %       ±7.00%  ±9.32% ±12.14%
streams/readable-encoding.js n=1000 op='unshift' len=512 encoding='latin1'                  -0.28 %       ±6.23%  ±8.30% ±10.85%
streams/readable-encoding.js n=1000 op='unshift' len=512 encoding='utf-8'                   -1.42 %       ±7.15%  ±9.51% ±12.39%

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[mcollina]

I don't link the decrease for latin1.

[anonrig]

@mcollina I updated the pr description with the CI result. My educated guess would be; its because of normalizeEncoding

[lpinca]

Can't we make Buffer.from() use the optimization used by TextDecoder or even use it directly?

[anonrig]

Can't we make Buffer.from() use the optimization used by TextDecoder or even use it directly?

We can't because Buffer.from returns Buffer instance not Uint8Array. As well as, TextDecoder uses string_decoder and Buffer uses a weird combination of StringSlice<T>

[anonrig]

I force pushed and fixed the tests. Will be running a new benchmark soon.

[jasnell]

Overall this change LGTM but, like @mcollina I am concerned about the regression on latin1. I'd like to see if there's a way to avoid that here.

[nodejs-github-bot]

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

[mcollina]

This change requires a bit more work due to readable-stream extraction that this will break due to the new dependencies:

FastBuffer and TextEncoder and normalizeEncoding might not be available. Could you put all that logic in a function imported from a different file? In this way it would be significantly easier to replace.

[anonrig]

It seems I forgot to add _read = noop() to the benchmark. Committed now with a fixup.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

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

Results

                                                                             confidence improvement accuracy (*)    (**)   (***)
streams/readable-encoding.js n=1000 op='push' len=16384 encoding='latin1'            **     -4.13 %       ±2.80%  ±3.72%  ±4.84%
streams/readable-encoding.js n=1000 op='push' len=16384 encoding='utf-8'            ***     18.80 %       ±3.57%  ±4.75%  ±6.18%
streams/readable-encoding.js n=1000 op='push' len=256 encoding='latin1'                     -3.49 %       ±6.33%  ±8.42% ±10.96%
streams/readable-encoding.js n=1000 op='push' len=256 encoding='utf-8'               **    -11.11 %       ±8.29% ±11.06% ±14.45%
streams/readable-encoding.js n=1000 op='push' len=512 encoding='latin1'                     -3.55 %       ±7.94% ±10.57% ±13.75%
streams/readable-encoding.js n=1000 op='push' len=512 encoding='utf-8'                *      7.64 %       ±6.77%  ±9.01% ±11.73%
streams/readable-encoding.js n=1000 op='unshift' len=16384 encoding='latin1'                -0.95 %       ±3.35%  ±4.46%  ±5.81%
streams/readable-encoding.js n=1000 op='unshift' len=16384 encoding='utf-8'         ***     18.21 %       ±3.95%  ±5.25%  ±6.83%
streams/readable-encoding.js n=1000 op='unshift' len=256 encoding='latin1'                  -5.41 %       ±5.63%  ±7.49%  ±9.75%
streams/readable-encoding.js n=1000 op='unshift' len=256 encoding='utf-8'           ***    -11.17 %       ±6.25%  ±8.32% ±10.84%
streams/readable-encoding.js n=1000 op='unshift' len=512 encoding='latin1'                  -6.10 %       ±8.53% ±11.36% ±14.82%
streams/readable-encoding.js n=1000 op='unshift' len=512 encoding='utf-8'                    4.06 %       ±7.18%  ±9.56% ±12.44%

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/45489
✔  Done loading data for nodejs/node/pull/45489
----------------------------------- PR info ------------------------------------
Title      stream: add fast-path for readable streams (#45489)
Author     Yagiz Nizipli  (@anonrig)
Branch     anonrig:perf/stream-encoding -> nodejs:main
Labels     stream, performance, needs-ci
Commits    2
 - stream: add fast-path for readable streams
 - fixup! stream: add fast-path for readable streams
Committers 1
 - Yagiz Nizipli 
PR-URL: https://github.com/nodejs/node/pull/45489
Reviewed-By: Antoine du Hamel 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45489
Reviewed-By: Antoine du Hamel 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup! stream: add fast-path for readable streams
   ℹ  This PR was created on Wed, 16 Nov 2022 19:23:56 GMT
   ✔  Approvals: 2
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45489#pullrequestreview-1187122865
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/45489#pullrequestreview-1187113060
   ✔  Last GitHub CI successful
   ℹ  Last Benchmark CI on 2022-11-21T18:10:58Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1228/
   ℹ  Last Full PR CI on 2022-11-21T18:10:00Z: https://ci.nodejs.org/job/node-test-pull-request/48072/
- Querying data for job/node-test-pull-request/1228/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3518963825

[aduh95]

The perf look worse for chunk of length 256, should we use the "fast-path" only when enc === 'utf8' && TypedArrayPrototypeGetByteLength(buf) > 512?

[anonrig]

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

[nodejs-github-bot]

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

[anonrig]

(Hopefully last) Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1243/

[mcollina]

This does not seems to be helping:

18:55:22                                                                              confidence improvement accuracy (*)    (**)   (***)
18:55:23 streams/readable-encoding.js n=1000 op='push' len=16384 encoding='latin1'                    1.26 %       ±2.69%  ±3.59%  ±4.67%
18:55:23 streams/readable-encoding.js n=1000 op='push' len=16384 encoding='utf-8'                    -1.97 %       ±3.26%  ±4.34%  ±5.65%
18:55:23 streams/readable-encoding.js n=1000 op='push' len=256 encoding='latin1'             ***    -14.39 %       ±6.91%  ±9.24% ±12.11%
18:55:23 streams/readable-encoding.js n=1000 op='push' len=256 encoding='utf-8'                      -7.88 %       ±8.53% ±11.36% ±14.80%
18:55:23 streams/readable-encoding.js n=1000 op='push' len=512 encoding='latin1'                     -7.25 %       ±7.68% ±10.23% ±13.31%
18:55:23 streams/readable-encoding.js n=1000 op='push' len=512 encoding='utf-8'                      -5.90 %       ±6.93%  ±9.24% ±12.06%
18:55:23 streams/readable-encoding.js n=1000 op='unshift' len=16384 encoding='latin1'                -0.23 %       ±3.24%  ±4.31%  ±5.62%
18:55:23 streams/readable-encoding.js n=1000 op='unshift' len=16384 encoding='utf-8'                  1.52 %       ±4.71%  ±6.28%  ±8.19%
18:55:23 streams/readable-encoding.js n=1000 op='unshift' len=256 encoding='latin1'           **     -9.98 %       ±7.00%  ±9.33% ±12.17%
18:55:23 streams/readable-encoding.js n=1000 op='unshift' len=256 encoding='utf-8'                   -6.96 %       ±7.97% ±10.61% ±13.82%
18:55:23 streams/readable-encoding.js n=1000 op='unshift' len=512 encoding='latin1'                  -0.69 %       ±6.82%  ±9.08% ±11.82%
18:55:23 streams/readable-encoding.js n=1000 op='unshift' len=512 encoding='utf-8'                   -4.19 %       ±7.09%  ±9.44% ±12.28%
18:55:23