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(transport/packet): don't (mutably) borrow data multiple times #34

Merged

Conversation

mxinden
Copy link

@mxinden mxinden commented Jan 26, 2025

Merges into mozilla#2385.

@larseggert
Copy link
Owner

LGTM.

I've been wondering if going with &mut slices into data for token, etc. would be a defensible choice for PublicPacket, since we then convert it into a non-&mut DecryptedPacket anyway. Maybe we should do that conversion earlier?

@mxinden
Copy link
Author

mxinden commented Jan 27, 2025

I am not sure I am following. Are you suggesting to split off dcid, scid and token off of &mut data earlier, thus make it a single immutable reference each?

Converting all of data into an immutable reference can only be done after decryption, correct? Otherwise it could never be in-place, i.e. back into data.

@larseggert
Copy link
Owner

Sorry for being unclear. If I understand it correctly, with this PR we're doing multiple (smaller) heap allocation for the various bits of the struct that can't be slices into data anymore. I wonder if that is actually worse. So I'm looking for ways to avoid those new allocations. One way would be to still let them be mut slices during parsing/decryption, but then make them non-mut slices afterwards, i.e., on DecryptedPacket.

@mxinden
Copy link
Author

mxinden commented Jan 28, 2025

Got it. Thanks.

Before optimizing these small allocations, I would suggest building a benchmark first, which shows that those allocations are significant.

Also note, ConnectionId uses a SmallVec. Unless the SmallVec limit is reached, the data goes on the stack, i.e. is not allocated on the heap.

@larseggert larseggert merged commit 3aff846 into larseggert:feat-inplace-crypto Jan 28, 2025
2 checks passed
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