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

Use std::list instead of std::deque for all buffers/queues in streams #3260

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jp4a50
Copy link
Collaborator

@jp4a50 jp4a50 commented Dec 19, 2024

API implementation objects.

A very common use case is to create one or more stream API objects per request. These streams currently have a high native memory overhead even when they are empty. Switching to std::list (which doesn't pre-allocate space for items) significantly alleviates this problem.

For example, now ReadableStreams tend to have <50% of their original memory footprint.

API implementation objects.

A very common use case is to create one or more stream API objects per
request. These streams currently have a high native memory overhead even
when they are empty. Switching to std::list (which doesn't pre-allocate
space for items) significantly alleviates this problem.

For example, now ReadableStreams tend to have <50% of their original
memory footprint.
@jp4a50 jp4a50 requested review from jasnell and danlapid December 19, 2024 14:36
@jp4a50 jp4a50 requested review from a team as code owners December 19, 2024 14:36
Copy link
Collaborator

@danlapid danlapid left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't know the streams codebase well enough.
Up to you on whether to wait for James with this or not.

@jp4a50
Copy link
Collaborator Author

jp4a50 commented Dec 19, 2024

I have a single concern about one of these conversions. We currently iterate through pendingByobReadRequests each time we call ByteQueue::maybeUpdateBackpressure which will likely now be appreciably slower. We currently call this on every Queue::push.

@jasnell thoughts on whether it is feasible/worth it to avoid doing this every time we call Queue::push?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants