Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: remove custom Buffer pool for streams #33981

Closed
wants to merge 3 commits into from
Closed

fs: remove custom Buffer pool for streams #33981

wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 19, 2020 •
edited 

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 ronag added the stream Issues and PRs related to the stream subsystem. label Jun 19, 2020
@ronag ronag requested a review from addaleax June 19, 2020 22:03
@nodejs-github-bot 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 ronag force-pushed the fs-pool branch 2 times, most recently from cf19ba7 to b58a7bb Compare June 19, 2020 22:04
@ronag ronag removed the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Jun 19, 2020
@ronag
Copy link
Member Author

ronag commented Jun 19, 2020

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

@ronag ronag mentioned this pull request Jun 20, 2020
Closed
bnoordhuis
bnoordhuis approved these changes Jun 20, 2020
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/internal/fs/streams.js Outdated Show resolved Hide resolved
@ronag 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
fs: remove custom Buffer pool for streams
2bc17a5 
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 ronag requested a review from jasnell June 20, 2020 08:41
@ronag 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
@ronag
Copy link
Member Author

ronag commented Jun 20, 2020

Needs a CI and benchmark CI

@addaleax 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
@ronag
Copy link
Member Author

ronag commented Jun 20, 2020 •
edited

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
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Jun 20, 2020

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
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Jun 21, 2020

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

@bnoordhuis bnoordhuis mentioned this pull request Jun 22, 2020
Closed
@bnoordhuis
Copy link
Member

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

@ronag
Copy link
Member Author

ronag commented Jun 22, 2020 •
edited

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
Copy link
Collaborator

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

@ronag 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
@ronag
Copy link
Member Author

ronag commented Jun 22, 2020 •
edited

@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?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 23, 2020 •
edited by ronag

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

@ronag 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
Copy link
Member Author

ronag commented Jun 23, 2020

Landed in db0e991

@ronag ronag closed this Jun 23, 2020
ronag added a commit that referenced this pull request Jun 23, 2020
@ronag
fs: remove custom Buffer pool for streams
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>
@addaleax addaleax mentioned this pull request Jun 27, 2020
Closed
2 tasks
@codebytere
Copy link
Member

@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 😅

@Zclhlmgqzc Zclhlmgqzc mentioned this pull request Oct 14, 2020
Closed
@gireeshpunathil gireeshpunathil mentioned this pull request Nov 4, 2020
Closed
@targos 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 pushed a commit to targos/node that referenced this pull request Apr 25, 2021
@ronag @targos
fs: remove custom Buffer pool for streams
0211c7a 
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>
@targos targos mentioned this pull request Apr 25, 2021
Closed
targos pushed a commit that referenced this pull request Apr 26, 2021
@ronag @targos
fs: remove custom Buffer pool for streams
443cace 
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>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
@richardlau richardlau mentioned this pull request Apr 6, 2023
Merged
nodejs-github-bot pushed a commit that referenced this pull request Apr 8, 2023
@richardlau
test: remove stale entry from known_issues.status
5ae2bce 
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>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
@richardlau @RafaelGSS
test: remove stale entry from known_issues.status
86f8905 
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>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@richardlau @danielleadams
test: remove stale entry from known_issues.status
dc65124 
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>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@richardlau @MoLow
test: remove stale entry from known_issues.status
095ca5c 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvagg rvagg rvagg left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fs,crypto: AAD decryption of fs stream > 32768 bytes fails
7 participants
@ronag @nodejs-github-bot @bnoordhuis @codebytere @rvagg @addaleax @targos