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

fs: remove custom Buffer pool for streams #33981

Closed
wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 19, 2020

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.

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
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
@nodejs-github-bot
Copy link
Collaborator

@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

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

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

@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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 23, 2020

@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
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@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 😅

@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
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Apr 26, 2021
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
nodejs-github-bot pushed a commit that referenced this pull request Apr 8, 2023
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 <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
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 <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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 <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
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 <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs,crypto: AAD decryption of fs stream > 32768 bytes fails
7 participants