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

perf: reduce amount of used Uin8Arrays in client-h1.js #3514

Closed
wants to merge 2 commits into from
Closed

perf: reduce amount of used Uin8Arrays in client-h1.js #3514

wants to merge 2 commits into from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Aug 26, 2024

instead of instantiating an Uint8Array on every llhttp execution, we keep a view on the ArrayBuffer and reuse it.

mcollina
mcollina reviewed Aug 27, 2024
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.

I have a rough memory that we tried this before but we had to revert it, because there were multiple http responses being written into the same buffer in parallel, and data got corrupted.

@ronag wdyt?

ronag
ronag reviewed Aug 27, 2024
View reviewed changes
lib/dispatcher/client-h1.js Outdated Show resolved Hide resolved
ronag
ronag approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I don't remember if this could cause problems. LGTM with the change i proposed.

@Uzlopak @ronag
Update lib/dispatcher/client-h1.js
b3c2bfc 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
ronag
ronag requested changes Aug 27, 2024
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

#788

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 27, 2024

hmm. yes. very strange. If I check the detached flag, it isnt set to true.

@Uzlopak Uzlopak closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina left review comments

@ronag ronag ronag requested changes

@metcoder95 metcoder95 metcoder95 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Uzlopak @mcollina @ronag @metcoder95