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: use mutable transmits to AsyncUdpSocket::poll_send #1692

Closed

Conversation

dignifiedquire
Copy link
Contributor

In our implementation (n0-computer/iroh) we currently have to maniuplate the destination addresses based on internal mappings. Currently we need to copy the full Transmit slice in order to update these addresses. To avoid this copy, this changes it such that a mutable reference is passed in.

In our implementation (n0-computer/iroh) we currently have to maniuplate the destination addresses based on internal mappings. Currently we need to copy the full `Transmit` slice in order to update these addresses. To avoid this copy, this changes it such that a mutable reference is passed in.
@Ralith
Copy link
Collaborator

Ralith commented Oct 20, 2023

What happens when a poll_send call isn't able to transmit all packets immediately, i.e. returns less than transmits.len()? It seems difficult to avoid unpredictably double-translating addresses with this change, which could be harmful if your translation isn't idempotent.

@dignifiedquire
Copy link
Contributor Author

That should be easy enough to handle on our side, we can either reset to the original addrs for those that weren't sent, or detect based on the addr if we have mapped it yet.

@dignifiedquire
Copy link
Contributor Author

The CI failures look unrelated to me: BSD seems to hang and the lint is complaining about unrelated code.

@chayleaf
Copy link

chayleaf commented Nov 24, 2023

this would be useful for us too (for address mangling as well)

@Ralith
Copy link
Collaborator

Ralith commented Nov 25, 2023

I'd like to at least see a clearly documented contract for whether or not unsent entries may be changed after the call returns. I think it should specifically guarantee that they won't be to avoid unpredictable behavior. It's not clear to me if that guarantee can be respected without defeating the point by requiring storage on the side, though.

Is this copy showing up in profiling at all? It should be straightforward to reuse an allocation for it.

@Ralith
Copy link
Collaborator

Ralith commented Dec 21, 2023

#1729 will likely render this unnecessary by only accepting a single, trivially rewritten Transmit at a time.

@djc djc mentioned this pull request Jan 12, 2024
7 tasks
@dignifiedquire
Copy link
Contributor Author

Closing in favor of #1729

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