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

feat(transport): accept borrowed instead of owned input datagram #2187

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Oct 17, 2024

Stacked on top of #2184.

Previously process_input (and the like) took a Datagram<Vec<u8>> as input. In other words, it required allocating each UDP datagram payload in a dedicated Vec<u8> before passing it to process_input.

With this patch, process_input accepts a Datagram<&[u8]>. In other words, it accepts a Datagram with a borrowed view into an existing buffer (&[u8]), e.g. a long lived receive buffer.


Extracted out of #2093.

Part of #1693.

Pull request stack:

  1. feat: don't allocate in UDP recv path #2184
  2. (this one) feat(transport): accept borrowed instead of owned input datagram #2187
  3. feat(udp): return borrowed Datagram on receive #2188
  4. feat(bin): don't allocate in UDP recv path #2189

neqo-bin/src/client/mod.rs Outdated Show resolved Hide resolved
neqo-http3/src/connection_client.rs Outdated Show resolved Hide resolved
neqo-transport/src/server.rs Outdated Show resolved Hide resolved
@mxinden
Copy link
Collaborator Author

mxinden commented Oct 21, 2024

Thanks for the input @martinthomson.

I applied your suggestion. process_xxx functions now take d: Datagram<impl AsRef<[u8]>>.

Given that there is no more need to pass a Datagram by reference, as one would instead pass Datagram<&[u8]>, this effectively reverts #1543.

I am sorry for the large diff. @martinthomson let me know whether this is the direction you were suggesting above.

Commits are still split into changes to neqo-common, neqo-transport (and neqo-http3), neqo-udp and neqo-bin.

Previously `process_input` (and the like) took a `Datagram<Vec<u8>>` as input.
In other words, it required allocating each UDP datagram payload in a dedicated
`Vec<u8>` before passing it to `process_input`.

With this patch, `process_input` accepts a `Datagram<&[u8]>`. In other words, it
accepts a `Datagram` with a borrowed view into an existing buffer (`&[u8]`),
e.g. a long lived receive buffer.
Previously `recv_inner` would return `Datagram<Vec<u8>>`. In other words, it
would allocate a new `Vec<u8>` for each UDP datagram payload.

Now `recv_inner` reads into a provided buffer and returns `Datagram<&[u8]>`,
i.e. it returns a view into the provided buffer without allocating.
Pass a long lived receive buffer to `neqo_udp::recv_inner`, receiving an
iterator of `Datagram<&[u8]>`s pointing into that buffer, thus no longer
allocating in UDP receive path.
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

This looks right. There's a lot of noise, but that just makes me want to land it faster.

I'm not 100% happy with the size of Datagrams being paired with having them passed by value, but I think that is something the compiler will manage for us mostly. (That is, SocketAddr is pretty big, and we have two of those, plus a byte, plus a slice. Copying that isn't super expensive, but marking that Copy means that the compiler won't help us avoid work that isn't constructive.) That said, I don't see any patterns here that are particularly problematic.

@larseggert larseggert merged commit 7c23ff6 into recv-no-alloc-common Oct 22, 2024
@larseggert larseggert deleted the recv-no-alloc-transport branch October 22, 2024 08:17
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.

3 participants