fs: reduce memory retention when streaming small files

[addaleax]

Fixes: #21967

/cc @nodejs/fs

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

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

[ChALkeR]

/cc @nodejs/benchmarking

[ChALkeR]

Changes look good, but lint fails.
Also, this probably needs benchmarking.

[addaleax]

CI: https://ci.nodejs.org/job/node-test-pull-request/16003/ (edit: green up to cancelled arm build)
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/218/

[addaleax]

Benchmark results:

                                                                                  confidence improvement accuracy (*)    (**)   (***)
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='asc'                   -0.88 %       ±4.57%  ±6.09%  ±7.92%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='buf'                    3.41 %       ±4.63%  ±6.16%  ±8.02%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='utf'                    0.92 %       ±2.74%  ±3.66%  ±4.79%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='asc'                 0.13 %       ±7.84% ±10.50% ±13.81%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='buf'                 0.21 %       ±7.09%  ±9.45% ±12.35%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='utf'          *    -12.78 %      ±10.99% ±14.66% ±19.15%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='asc'                    1.49 %       ±3.29%  ±4.38%  ±5.72%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='buf'                    0.86 %       ±4.67%  ±6.22%  ±8.12%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='utf'                   -7.13 %       ±8.20% ±10.91% ±14.22%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='asc'                   0.07 %       ±6.29%  ±8.38% ±10.91%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='buf'                  -1.41 %       ±7.39%  ±9.83% ±12.81%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='utf'                   4.10 %      ±12.59% ±16.75% ±21.81%

Since there really isn’t any reason why the encoding should matter here, and we should expect 0.6 false positives, I’d ignore the one * result. If somebody feels like we should make sure, feel free to run the benchmarks with more iterations.

[mcollina]

LGTM.

Could this be effecting also 'net'?

[addaleax]

@mcollina You mean, the same bug there? No, that’s not the case, because we shrink the buffer in native land before handing it over to JS. I’ve thought about implementing similar pooling there, though, as part of looking into https://github.com/nodejs-private/security/issues/186 (for those who can’t see it: it’s the bug that was resolved in 3217e8e).

[mcollina]

@addaleax Thx! there are a few cases in HTTP applications where I have seen very high RSS vs heap usage, without a leak. The effect looks very similar to this one.

[addaleax]

ARM build: https://ci.nodejs.org/job/node-test-commit-arm-fanned/2505/

[addaleax]

Landed in e3a4702

[ChALkeR]

/cc @nodejs/lts ?