test: relax chunk count expectations

[gireeshpunathil]

In parallel/test-fs-read-stream-concurrent-reads.js the number of data chunks used is being tested when few concurrent reads are performed. The number of chunks can fluctuate based on the
number of concurrent reads as well as the data that was read in one shot. Accommodate these variations in the test.

Rational for M * 2 in the assertion statement:
Number of chunks are clearly a function of the (for) loop iteration, and hence M
Number of chunks are rarely increase based on the number of iterations it took complete one read. Mostly one, rarely 2. Accommodate this fluctuation by taking the worst case situation: all the reads take 2 iterations. And hence M * 2.

Fixes: #22339

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

ping @nodejs/testing @addaleax @Trott

[nodejs-github-bot]

@gireeshpunathil build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2204/pipeline

[gireeshpunathil]

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

[gireeshpunathil]

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20100/

[addaleax]

I do not understand the PR description, tbh … the number of chunks should not be as relevant as the number of buffers – and as we are trying to re-use buffers as much as possible in the ReadStream implementation, I don’t think an 8× memory overhead would be within expected limits?

[gireeshpunathil]

sorry if I was vague. Here is the proof:

index 32a6cd6..9b3af76 100644
--- a/test/parallel/test-fs-read-stream-concurrent-reads.js
+++ b/test/parallel/test-fs-read-stream-concurrent-reads.js
@@ -14,6 +14,7 @@ const filename = fixtures.path('loop.js');  // Some small non-homogeneous file.
 const content = fs.readFileSync(filename);
 
 const N = 1000;
+const M = process.argv[2] / 1;
 let started = 0;
 let done = 0;
 
@@ -34,14 +35,15 @@ function startRead() {
       if (++done === N) {
         const retainedMemory =
           [...arrayBuffers].map((ab) => ab.byteLength).reduce((a, b) => a + b);
-        assert(retainedMemory / (N * content.length) <= 3,
-               `Retaining ${retainedMemory} bytes in ABs for ${N} ` +
-               `chunks of size ${content.length}`);
+        console.log(`growth: ${arrayBuffers.size}, concurrency: ${M}`)
+        // assert(retainedMemory / (N * content.length) <= 3,
+        //        `Retaining ${retainedMemory} bytes in ABs for ${N} ` +
+        //       `chunks of size ${content.length}`);
       }
     }));
 }
 
 // Don’t start the reads all at once – that way we would have to allocate
 // a large amount of memory upfront.
-for (let i = 0; i < 4; ++i)
+for (let i = 0; i < M; ++i)
   startRead();

$  ./node test/parallel/test-fs-read-stream-concurrent-reads.js 4
growth: 5, concurrency: 4
$  ./node test/parallel/test-fs-read-stream-concurrent-reads.js 5
growth: 6, concurrency: 5
$  ./node test/parallel/test-fs-read-stream-concurrent-reads.js 10
growth: 11, concurrency: 10
$  ./node test/parallel/test-fs-read-stream-concurrent-reads.js 100
growth: 101, concurrency: 100
$

so clearly the number of buffers (64K entries in the set) is a function of the concurrent reads, and hence I associated the predicate with that, labelling as M.

M * 2 => is where probably we can argue: For every read, if the actual file read took 2 steps instead of one (for unknown reasons related to stream internal logic, file system logic, load on the system etc.) then 2 buffers are allocated instead of one.

[gireeshpunathil]

ok, I got your response in the main issue; so will have a look and follow your suggestion, thanks.

[addaleax]

@gireeshpunathil Okay, I think we basically figured out the same things :) So yeah, the issue is that we have too many concurrent reads for too little actual content.

[addaleax]

(We may want to think about a strategy to fix this in the stream implementation itself, btw – It’s not great that 100 concurrent reads allocate 100 full-sized buffers. I’m not sure how to work around that, though.)

[gireeshpunathil]

@addaleax - pushed a change to that effect; ptal. minor deviation is in terms of N :

N = (pool size) * (concurrent reads) / (file size)
  = 65536 * 6 / 170
  = 2313

so I took 2000 on the safer side.

[addaleax]

LGTM :)

[addaleax]

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

[gireeshpunathil]

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20150/

[gireeshpunathil]

landed as cc26957