fs: remove custom Buffer pool for streams by ronag · Pull Request #33981 · nodejs/node

ronag · 2020-06-19T22:03:35Z

The performance benefit of using a custom pool are negligable.
Furthermore, it causes problems with Workers and transferrable.
Rather than further adding complexity for compat with Workers,
just remove the pooling logic.

Refs: #33880 (comment)
Fixes: #31733

Checklist

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

ronag · 2020-06-19T22:07:18Z

Needs a CI and benchmark CI... However, at the moment I'm unable to start either for some reason. Will try again tomorrow.

bnoordhuis

Can you also update the comment on line 134 in test-crypto-authenticated-stream.js?

lib/internal/fs/streams.js

The performance benefit of using a custom pool are negligable. Furthermore, it causes problems with Workers and transferrable. Rather than further adding complexity for compat with Workers, just remove the pooling logic. Refs: nodejs#33880 (comment) Fixes: nodejs#31733

ronag · 2020-06-20T12:02:44Z

Needs a CI and benchmark CI

ronag · 2020-06-20T15:34:57Z

https://ci.nodejs.org/job/node-test-pull-request/31988/
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/620/

nodejs-github-bot · 2020-06-20T20:58:06Z

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

ronag · 2020-06-20T20:59:24Z

Slower with a low highwatermark < 4k, which is expected and I think that's acceptable as I don't see that being a common case at all.

19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='asc'           ***    -11.00 %       ±3.01%  ±4.01%  ±5.22%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='buf'           ***    -10.15 %       ±3.46%  ±4.61%  ±6.01%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='utf'           ***     -6.08 %       ±2.53%  ±3.37%  ±4.40%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='asc'                 1.28 %       ±4.14%  ±5.52%  ±7.20%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='buf'          *     14.52 %      ±13.09% ±17.42% ±22.67%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='utf'                17.52 %      ±20.33% ±27.07% ±35.28%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='asc'                   -0.39 %       ±2.94%  ±3.91%  ±5.10%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='buf'             *     -4.47 %       ±3.36%  ±4.47%  ±5.83%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='utf'                   -1.49 %       ±2.29%  ±3.05%  ±3.97%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='asc'                  -1.46 %       ±8.08% ±10.75% ±13.99%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='buf'                  -0.84 %       ±7.46%  ±9.94% ±12.96%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='utf'                  -0.54 %       ±6.16%  ±8.19%

nodejs-github-bot · 2020-06-21T08:33:46Z

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

ronag · 2020-06-21T10:09:22Z

@nodejs/crypto: Got problems with two crypto failures https://ci.nodejs.org/job/node-test-commit-linux-containered/20895/ Any ideas?

bnoordhuis · 2020-06-22T10:36:16Z

@ronag I've opened nodejs/build#2363 for the setAAD issue. Didn't the crypto check fix the other test failure?

ronag · 2020-06-22T10:52:52Z

Didn't the crypto check fix the other test failure?

Doesn't seem so. Which confuses me. I'll try another CI.

nodejs-github-bot · 2020-06-22T10:55:07Z

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

ronag · 2020-06-22T12:00:42Z

@bnoordhuis: Still failing... throw new ERR_NO_CRYPTO(); https://ci.nodejs.org/job/node-test-commit-linux-containered/20915/nodes=ubuntu1804_sharedlibs_withoutssl_x64/testReport/junit/(root)/test/parallel_test_crypto_authenticated_stream/

Do I need to do anything further than ca1d829?

test/parallel/test-crypto-authenticated-stream.js

nodejs-github-bot · 2020-06-23T09:59:08Z

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

ronag · 2020-06-23T11:27:18Z

Landed in db0e991

The performance benefit of using a custom pool are negligable. Furthermore, it causes problems with Workers and transferrable. Rather than further adding complexity for compat with Workers, just remove the pooling logic. Refs: #33880 (comment) Fixes: #31733 PR-URL: #33981 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

codebytere · 2020-06-27T19:13:19Z

@ronag could you please backport this to v14.x? It conflicted in a handful of places and I didn't want to try my luck 😅

The performance benefit of using a custom pool are negligable. Furthermore, it causes problems with Workers and transferrable. Rather than further adding complexity for compat with Workers, just remove the pooling logic. Refs: nodejs#33880 (comment) Fixes: nodejs#31733 PR-URL: nodejs#33981 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The performance benefit of using a custom pool are negligable. Furthermore, it causes problems with Workers and transferrable. Rather than further adding complexity for compat with Workers, just remove the pooling logic. Refs: #33880 (comment) Fixes: #31733 PR-URL: #33981 Backport-PR-URL: #38397 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The `test-crypto-authenticated-stream` test was moved out of `test/known_issues` and now lives in `test/parallel` PR-URL: #47454 Refs: #33981 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

The `test-crypto-authenticated-stream` test was moved out of `test/known_issues` and now lives in `test/parallel` PR-URL: nodejs#47454 Refs: nodejs#33981 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

ronag added the stream Issues and PRs related to the stream subsystem. label Jun 19, 2020

ronag requested a review from addaleax June 19, 2020 22:03

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Jun 19, 2020

ronag force-pushed the fs-pool branch 2 times, most recently from cf19ba7 to b58a7bb Compare June 19, 2020 22:04

addaleax approved these changes Jun 19, 2020

View reviewed changes

ronag removed the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Jun 19, 2020

ronag mentioned this pull request Jun 20, 2020

fs,crypto: AAD decryption of fs stream > 32768 bytes fails #31733

Closed

ronag force-pushed the fs-pool branch from b58a7bb to 4300f74 Compare June 20, 2020 08:18

bnoordhuis approved these changes Jun 20, 2020

View reviewed changes

lib/internal/fs/streams.js Outdated Show resolved Hide resolved

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2020

ronag force-pushed the fs-pool branch from b961333 to 2bc17a5 Compare June 20, 2020 08:41

ronag requested a review from jasnell June 20, 2020 08:41

ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2020

addaleax added needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. labels Jun 20, 2020

fixup

ca1d829

bnoordhuis mentioned this pull request Jun 22, 2020

Upgrade openssl to 1.1.1g on ubuntu1804_sharedlibs_openssl111_x64 nodejs/build#2363

Closed

ronag removed needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. labels Jun 22, 2020

rvagg reviewed Jun 23, 2020

View reviewed changes

test/parallel/test-crypto-authenticated-stream.js Outdated Show resolved Hide resolved

fixup

2facf66

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 23, 2020

ronag closed this Jun 23, 2020

addaleax mentioned this pull request Jun 27, 2020

tools: update ESLint from 7.2.0 to 7.3.0 #34000

Closed

2 tasks

codebytere added the backport-requested-v14.x label Jun 27, 2020

Zclhlmgqzc mentioned this pull request Oct 14, 2020

[Bug report] fs.createReadStream set highWaterMark, the same file get different result #35465

Closed

gireeshpunathil mentioned this pull request Nov 4, 2020

worker: motivating examples? #33880

Closed

targos added backport-open-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v14.x labels Apr 25, 2021

targos mentioned this pull request Apr 25, 2021

[v14.x] fs: remove custom Buffer pool for streams #38397

Closed

targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 26, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

richardlau mentioned this pull request Apr 6, 2023

test: remove stale entry from known_issues.status #47454

Merged

Uh oh!

Conversation

ronag commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Jun 19, 2020

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronag commented Jun 20, 2020

Uh oh!

ronag commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 20, 2020

Uh oh!

ronag commented Jun 20, 2020

Uh oh!

nodejs-github-bot commented Jun 21, 2020

Uh oh!

ronag commented Jun 21, 2020

Uh oh!

bnoordhuis commented Jun 22, 2020

Uh oh!

ronag commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 22, 2020

Uh oh!

ronag commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 23, 2020 • edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jun 23, 2020

Uh oh!

codebytere commented Jun 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ronag commented Jun 19, 2020 •

edited

Loading

ronag commented Jun 20, 2020 •

edited

Loading

ronag commented Jun 22, 2020 •

edited

Loading

ronag commented Jun 22, 2020 •

edited

Loading

nodejs-github-bot commented Jun 23, 2020 •

edited by ronag

Loading