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.

Sign up for GitHub

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

stream: use ByteLengthQueuingStrategy when not in object mode #48847

Merged
merged 14 commits into from
May 12, 2024
Merged

stream: use ByteLengthQueuingStrategy when not in object mode #48847

merged 14 commits into from
May 12, 2024

Conversation

CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Jul 20, 2023 •
edited
Loading

Use ByteLengthQueuingStrategy when not in object mode

After this PR

Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
converting node stream to web stream
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
reading the chunks
Array buffers memory usage is 2 MiB
Array buffers memory usage is 31 MiB
Array buffers memory usage is 9 MiB
Array buffers memory usage is 18 MiB
Array buffers memory usage is 9 MiB
Array buffers memory usage is 5 MiB
Array buffers memory usage is 8 MiB
Array buffers memory usage is 17 MiB
Array buffers memory usage is 13 MiB
Array buffers memory usage is 12 MiB
Array buffers memory usage is 19 MiB
Array buffers memory usage is 20 MiB
Array buffers memory usage is 31 MiB
Array buffers memory usage is 11 MiB
Array buffers memory usage is 24 MiB
Array buffers memory usage is 17 MiB

fixes: #46347

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jul 20, 2023
@CGQAQ CGQAQ changed the title stream: don't pull for first time (fixes #46347) stream: don't pull for the first time (fixes #46347) Jul 20, 2023
@debadree25
Copy link
Member

debadree25 commented Jul 20, 2023 •
edited
Loading

This would probably not be spec compliant, https://streams.spec.whatwg.org/#readable-stream-default-controller-should-call-pull I do not think we are allowed to add additional steps not mentioned in the spec

@CGQAQ CGQAQ requested a review from debadree25 July 21, 2023 02:06
@CGQAQ CGQAQ changed the title stream: don't pull for the first time (fixes #46347) stream: fix wrong stream type in adapters.js (fixes nodejs#46347) Jul 21, 2023
@CGQAQ

This comment was marked as resolved.

Sign in to view
@CGQAQ CGQAQ changed the title stream: fix wrong stream type in adapters.js (fixes nodejs#46347) stream: use ByteLengthQueuingStrategy when not in object mode Jul 21, 2023
@CGQAQ CGQAQ force-pushed the stream-nodestream-toweb branch 2 times, most recently from c5c66b4 to 81c3376 Compare July 21, 2023 09:52
@debadree25
Copy link
Member

A previous PR had attempted the same, nonethless would cc @nodejs/whatwg-stream to take a look!

@jasnell
Copy link
Member

jasnell commented Sep 14, 2023

This would probably not be spec compliant, streams.spec.whatwg.org/#readable-stream-default-controller-should-call-pull I do not think we are allowed to add additional steps not mentioned in the spec

Given that this is for our Node.js stream adapters, this change should be fine.

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 15, 2023
Open
13 tasks
@nodejs-github-bot
Copy link
Collaborator

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

debadree25
debadree25 reviewed Sep 15, 2023
View reviewed changes
test/parallel/test-stream-readable-to-web.js

assert(currentMemoryUsage <= 256 * 1024 * 1024);
};
setInterval(reportMemoryUsage, 1000);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this test could be somewhat flakey? too many timeouts but nonetheless shouldn't be a blocker

mcollina
mcollina approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@debadree25 debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 15, 2023
This was referenced Sep 16, 2023
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 24, 2023
Open
20 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 merged commit 261e88e into nodejs:main May 12, 2024
53 checks passed
@aduh95
Copy link
Contributor

aduh95 commented May 12, 2024

Landed in 261e88e

targos pushed a commit that referenced this pull request May 12, 2024
@CGQAQ @targos
stream: use ByteLengthQueuingStrategy when not in objectMode
8d3b0b7 
Fixes: #46347
PR-URL: #48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@CGQAQ CGQAQ deleted the stream-nodestream-toweb branch May 13, 2024 02:33
@targos targos mentioned this pull request May 13, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@CGQAQ @marco-ippolito
stream: use ByteLengthQueuingStrategy when not in objectMode
6257273 
Fixes: #46347
PR-URL: #48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@CGQAQ @marco-ippolito
stream: use ByteLengthQueuingStrategy when not in objectMode
4010424 
Fixes: #46347
PR-URL: #48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@CGQAQ @marco-ippolito
stream: use ByteLengthQueuingStrategy when not in objectMode
af29120 
Fixes: #46347
PR-URL: #48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
@CGQAQ @EliphazBouye
stream: use ByteLengthQueuingStrategy when not in objectMode
4aee175 
Fixes: nodejs#46347
PR-URL: nodejs#48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@CGQAQ @bmeck
stream: use ByteLengthQueuingStrategy when not in objectMode
a7e3f47 
Fixes: nodejs#46347
PR-URL: nodejs#48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@arsnyder16
Copy link

@CGQAQ Same issues with newWritableStreamFromStreamWritable ?https://github.com/CGQAQ/node/blob/885692a34f582c99d74430ba351879431738e43a/lib/internal/webstreams/adapters.js#L167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@debadree25 debadree25 debadree25 left review comments

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Readable.toWeb seems to load file contents to memory
9 participants
@CGQAQ @debadree25 @jasnell @nodejs-github-bot @aduh95 @idranme @mcollina @StefanStojanovic @arsnyder16