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: Improve performance of Exchange #104

Merged
merged 10 commits into from
Oct 16, 2023

Conversation

oblique
Copy link
Member

@oblique oblique commented Oct 13, 2023

We sacrifice memory usage for performance.

We sacrifice memory usage for performance.
@oblique oblique requested review from fl0rek and zvolin October 13, 2023 12:39
Copy link
Member

@zvolin zvolin left a comment

Choose a reason for hiding this comment

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

cool 🔥

node/src/exchange.rs Outdated Show resolved Hide resolved
Co-authored-by: Maciej Zwoliński <mac.zwolinski@gmail.com>
Signed-off-by: Yiannis Marangos <psyberbits@gmail.com>
Copy link
Member

@zvolin zvolin left a comment

Choose a reason for hiding this comment

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

Miri has no issues with it but I'm concerned about the transmute.

In the MaybeUninit docs there is this example (3rd in section)
https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initialization-invariant

let x: i32 = unsafe { MaybeUninit::uninit().assume_init() }; // undefined behavior! ⚠️

so I think it would also do so for

let x: MaybeUninit<u8> = MaybeUninit::uninit();
let x: u8 = std::mem::transmute(x); // ub

also I found this MaybeUninit::slice_assume_init_mut which happens to do the same thing and it says that calling is unsafe on uninitialized data
https://doc.rust-lang.org/src/core/mem/maybe_uninit.rs.html#976

Want to open a discussion what do we want to do with it. It seems it's safe by current compiler's behavior and used in some popular crates but causes UB per documentation.

@oblique
Copy link
Member Author

oblique commented Oct 13, 2023

I posted a question about this in the Rust forum (link) and the code is safe as long as Stream only fills the slice. If Stream::poll_read try to read a byte from the slice that wasn't filled yet, then it is UB.

I have checked all the Stream types we use (tokio-tcp, noise, yamux, quic) and they only fill the slice. But for the sake of peace of mind for future releases of the mentioned types, I'm going to remove the unsafe. By removing a ~1ms of overhead will be added.

@oblique oblique requested a review from zvolin October 14, 2023 08:34
Copy link
Member

@zvolin zvolin left a comment

Choose a reason for hiding this comment

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

Nice

Copy link
Member

@fl0rek fl0rek left a comment

Choose a reason for hiding this comment

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

great work! 🚀

@fl0rek fl0rek merged commit c3e5838 into eigerco:main Oct 16, 2023
4 checks passed
@github-actions github-actions bot mentioned this pull request Jan 9, 2024
@zvolin zvolin mentioned this pull request Jan 12, 2024
@oblique oblique deleted the feat/improve-req-resp branch April 17, 2024 14:02
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