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

fix: copy buffers #711

Merged
merged 1 commit into from
Apr 9, 2021
Merged

fix: copy buffers #711

merged 1 commit into from
Apr 9, 2021

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 9, 2021

Fixes: #710

@ronag
Copy link
Member Author

ronag commented Apr 9, 2021

@indutny

@ronag ronag requested review from mcollina and dnlup April 9, 2021 05:42
@ronag
Copy link
Member Author

ronag commented Apr 9, 2021

Not sure if there is a faster way to copy which doesn't not involved creating a temporary Buffer.

@indutny
Copy link
Member

indutny commented Apr 9, 2021

I think this should be about as fast as it can be, but you could experiment by creating a buffer from buffer something like wrapping your previous Buffer.from() calls with another Buffer.from(). The benefit of latter is that you don't do unsafe allocations.

@ronag
Copy link
Member Author

ronag commented Apr 9, 2021

@indutny: Another question. Would it be possible for wasm_on_header_ and wasm_on_body to instead point into the buffer passed to execute? That way no copy would be required. The exact same data should already be there, or alternatively could be written there.

@ronag ronag mentioned this pull request Apr 9, 2021
@indutny
Copy link
Member

indutny commented Apr 9, 2021

@ronag that buffer is relative to the input of the execute already. I think the problem is that you copy it into wasm memory and so you get back a slice of the very same memory you copied it to. Not sure if there is a way to pass data to wasm without copying. Perhaps shared buffers?

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 feeling that this unwinds any gain we got by using buffers for Headers, would not be better to revert?

Anyway LGTM

@ronag
Copy link
Member Author

ronag commented Apr 9, 2021

I have a feeling that this unwinds any gain we got by using buffers for Headers, would not be better to revert?

Anyway LGTM

I still haven’t given up. I think I can sort this as zero copy still.

@ronag ronag merged commit 68432eb into main Apr 9, 2021
ronag added a commit that referenced this pull request Apr 9, 2021
ronag added a commit that referenced this pull request Apr 9, 2021
* perf: zero copy llhttp buffers

Fixes: nodejs/llhttp#100
Refs: #711

* fixup

* fixup

* fixup
@Uzlopak Uzlopak deleted the copy-buf branch February 21, 2024 12:38
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* perf: zero copy llhttp buffers

Fixes: nodejs/llhttp#100
Refs: nodejs#711

* fixup

* fixup

* fixup
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.

Weird test flakiness
3 participants