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)