stream: remove Array.p.shift primordial from hotpath of RS.read()

[debadree25]

Refs: nodejs/performance#82

A small improvement noticed in performance of read():

                                                   confidence improvement accuracy (*)   (**)  (***)
webstreams/readable-async-iterator.js n=100000                     0.75 %       ±1.93% ±2.57% ±3.35%
webstreams/readable-read.js type='byob' n=100000                   0.69 %       ±1.21% ±1.61% ±2.10%
webstreams/readable-read.js type='normal' n=100000        ***      8.81 %       ±1.55% ±2.07% ±2.70%

[debadree25]

cc @nodejs/performance @nodejs/whatwg-stream

[anonrig]

cc @nodejs/primordials

[nodejs-github-bot]

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

[debadree25]

benchmark ci https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1453/ (hope i started it correctly 😅)

[mcollina]

lgtm

[ljharb]

Since shift is O(n), rather than removing a primordial, could this be made more performant by conceptually reversing the array throughout, so that this shift operation can become arr[arr.length - 1]; arr.length -= 1;?

[debadree25]

Since shift is O(n), rather than removing a primordial, could this be made more performant by conceptually reversing the array throughout, so that this shift operation can become arr[arr.length - 1]; arr.length -= 1;?

Can give that a try in a follow up although dont you think that might break the spec? i feel like it might, will report back with an investigation 👍

[ljharb]

I'm hoping/assuming it's an internal implementation detail, but I look forward to your attempt!

[debadree25]

Benchmark CI results:

23:08:36 webstreams/creation.js kind='ReadableStream.tee' n=50000                               1.81 %       ±2.93% ±3.89%  ±5.07%
23:08:36 webstreams/creation.js kind='ReadableStream' n=50000                                   1.54 %       ±4.85% ±6.46%  ±8.43%
23:08:36 webstreams/creation.js kind='ReadableStreamBYOBReader' n=50000                         0.16 %       ±6.40% ±8.52% ±11.09%
23:08:36 webstreams/creation.js kind='ReadableStreamDefaultReader' n=50000                     -3.95 %       ±5.36% ±7.14%  ±9.32%
23:08:36 webstreams/creation.js kind='TransformStream' n=50000                                 -3.64 %       ±3.77% ±5.02%  ±6.54%
23:08:36 webstreams/creation.js kind='WritableStream' n=50000                                   1.90 %       ±4.71% ±6.27%  ±8.18%
23:08:36 webstreams/js_transfer.js n=10000 payload='ReadableStream'                             1.45 %       ±1.76% ±2.34%  ±3.05%
23:08:36 webstreams/js_transfer.js n=10000 payload='TransformStream'                            1.14 %       ±1.33% ±1.77%  ±2.30%
23:08:36 webstreams/js_transfer.js n=10000 payload='WritableStream'                            -0.66 %       ±1.73% ±2.31%  ±3.00%
23:08:36 webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=1024 n=500000                 0.72 %       ±2.01% ±2.68%  ±3.49%
23:08:36 webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=2048 n=500000                -0.44 %       ±1.90% ±2.53%  ±3.30%
23:08:36 webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=4096 n=500000                -0.26 %       ±2.03% ±2.70%  ±3.51%
23:08:36 webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=512 n=500000                 -0.91 %       ±2.16% ±2.88%  ±3.76%
23:08:36 webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=1024 n=500000                 0.97 %       ±1.79% ±2.38%  ±3.10%
23:08:36 webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=2048 n=500000                 1.12 %       ±1.47% ±1.96%  ±2.55%
23:08:36 webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=4096 n=500000                 0.95 %       ±1.82% ±2.42%  ±3.15%
23:08:36 webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=512 n=500000                  0.75 %       ±2.23% ±2.97%  ±3.87%
23:08:36 webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=1024 n=500000                 1.17 %       ±1.87% ±2.48%  ±3.23%
23:08:36 webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=2048 n=500000                 0.07 %       ±2.04% ±2.71%  ±3.52%
23:08:36 webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=4096 n=500000                 0.32 %       ±1.79% ±2.39%  ±3.11%
23:08:36 webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=512 n=500000                 -0.72 %       ±2.17% ±2.89%  ±3.76%
23:08:36 webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=1024 n=500000                 -0.59 %       ±1.88% ±2.50%  ±3.25%
23:08:36 webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=2048 n=500000                 -1.26 %       ±2.06% ±2.74%  ±3.57%
23:08:36 webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=4096 n=500000                  0.01 %       ±2.24% ±2.98%  ±3.87%
23:08:36 webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=512 n=500000                   0.32 %       ±2.37% ±3.16%  ±4.13%
23:08:36 webstreams/readable-async-iterator.js n=100000                                        -1.20 %       ±3.09% ±4.16%  ±5.49%
23:08:36 webstreams/readable-read.js type='byob' n=100000                                *      3.50 %       ±2.83% ±3.76%  ±4.90%
23:08:36 webstreams/readable-read.js type='normal' n=100000                              *      8.67 %       ±6.71% ±8.93% ±11.65%

[nodejs-github-bot]

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

[targos]

Benchmark CI results aren't really convincing: two small positive results when we should expect 1.4 false positives

[debadree25]

two small positive results when we should expect 1.4 false positives

Didn't get you 😅 could you explain?

edit: oh ok so this could be within statistical range of error? hmm it seems i was able to get higher confidence running it locally 🤔

[aduh95]

oh ok so this could be within statistical range of error? hmm it seems i was able to get higher confidence running it locally 🤔

Maybe you are running the benchmark on a different platform (CPU arch, OS, etc.) than our CI, so that means either that change does not affect the benchmark CI, or either yours or the CI's result is a statistical error. Re-running the benchmarks with more data points would be a way to settle the question, but I would encourage you to investigate #50340 (comment) first, as there's a potential to get a greater perf win while preserving the robustness of the code.

[debadree25]

i am investtigating #50340 (comment) turning this into draft for now

[debadree25]

Hmm so i tried inverting the array doesnt seem to yield any performance benefits, tried something like this

diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js
index a5f95596e3..f7dc3157ff 100644
--- a/lib/internal/webstreams/readablestream.js
+++ b/lib/internal/webstreams/readablestream.js
@@ -6,6 +6,7 @@ const {
   ArrayBufferPrototypeSlice,
   ArrayPrototypePush,
   ArrayPrototypeShift,
+  ArrayPrototypeUnshift,
   DataView,
   FunctionPrototypeBind,
   FunctionPrototypeCall,
@@ -2082,7 +2083,8 @@ function readableStreamFulfillReadRequest(stream, chunk, done) {
     reader,
   } = stream[kState];
   assert(reader[kState].readRequests.length);
-  const readRequest = ArrayPrototypeShift(reader[kState].readRequests);
+  const readRequest = reader[kState].readRequests[reader[kState].readRequests.length - 1];
+  reader[kState].readRequests.length -= 1;
 
   // TODO(@jasnell): It's not clear under what exact conditions done
   // will be true here. The spec requires this check but none of the
@@ -2110,7 +2112,7 @@ function readableStreamFulfillReadIntoRequest(stream, chunk, done) {
 function readableStreamAddReadRequest(stream, readRequest) {
   assert(readableStreamHasDefaultReader(stream));
   assert(stream[kState].state === 'readable');
-  ArrayPrototypePush(stream[kState].reader[kState].readRequests, readRequest);
+  stream[kState].reader[kState].readRequests = [readRequest, ...stream[kState].reader[kState].readRequests];
 }
 
 function readableStreamAddReadIntoRequest(stream, readIntoRequest) {
@@ -3156,9 +3158,11 @@ function readableByteStreamControllerProcessReadRequestsUsingQueue(controller) {
     if (queueTotalSize === 0) {
       return;
     }
+    const shifted = reader[kState].readRequests[reader[kState].readRequests.length - 1];
+    reader[kState].readRequests.length -= 1;
     readableByteStreamControllerFillReadRequestFromQueue(
       controller,
-      ArrayPrototypeShift(reader[kState].readRequests),
+      shifted,
     );
   }
 }

and the local benchmark gives:

                                                   confidence improvement accuracy (*)   (**)  (***)
webstreams/readable-async-iterator.js n=100000              *     -1.41 %       ±1.27% ±1.70% ±2.22%
webstreams/readable-read.js type='byob' n=100000                   0.11 %       ±2.14% ±2.84% ±3.70%
webstreams/readable-read.js type='normal' n=100000         **      1.47 %       ±1.03% ±1.37% ±1.78%

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

[benjamingr]

fwiw the correct data structure for this is probably a deque

[ljharb]

That array spread will kill speed, what if you use concat for that?

[debadree25]

That array spread will kill speed, what if you use concat for that?

With concat stream[kState].reader[kState].readRequests = [readRequest].concat(stream[kState].reader[kState].readRequests);

                                                   confidence improvement accuracy (*)   (**)   (***)
webstreams/readable-async-iterator.js n=100000                     2.26 %       ±5.33% ±7.10%  ±9.27%
webstreams/readable-read.js type='byob' n=100000                   1.35 %       ±2.98% ±3.97%  ±5.18%
webstreams/readable-read.js type='normal' n=100000        ***    -12.67 %       ±6.52% ±8.70% ±11.36%

fwiw the correct data structure for this is probably a deque

This is interesting a linkedlist based deque could work here I guess!

[debadree25]

Is there any internal implementation of a deque that can be reused? @benjamingr

[rluvaton]

Is there any internal implementation of a deque that can be reused?

There was BufferList that @ronag just removed:

• https://github.com/nodejs/node/pull/50341/files#diff-92d05bac653ffcdc11f6b22f18986122a4101816c037c57d768f8579c978a5c7

I had a PR to change from array to BufferList though and it did not have any change IIRC

also, @ronag moved away from it, and it made it faster

[debadree25]

Ahh got it hmmm I havent gotten time yet to test but it does seem that doing array prototype shift frequently cases a bit of perf issue theres even an issue regarding it https://bugs.chromium.org/p/v8/issues/detail?id=12730

[ljharb]

In your example, if you use unshift instead of concat, what do the benchmarks look like?

[debadree25]

Similarish results as #50340 (comment)

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.