-
-
Notifications
You must be signed in to change notification settings - Fork 372
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
[SCTP] Optimize sctp packet marshalling #364
[SCTP] Optimize sctp packet marshalling #364
Conversation
Codecov ReportBase: 59.99% // Head: 59.99% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #364 +/- ##
==========================================
- Coverage 59.99% 59.99% -0.01%
==========================================
Files 504 504
Lines 48073 48068 -5
Branches 12517 12517
==========================================
- Hits 28839 28836 -3
+ Misses 10011 10010 -1
+ Partials 9223 9222 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-authored-by: Anton <[email protected]>
Just an FYI, I implemented a faster crc32 algorithm on the merged branch in my repo just as an PoC that make the crc calucations basically disappear in the profile and increases throughput even more. I'll try to port this to the crc crate to benefit the whole ecosystem. If they don't want it I'll make an PR to at least let this crate benefit :) |
if padding_needed != 0 { | ||
raw.extend(vec![0u8; padding_needed]); | ||
// padding needed if < 4 because we pad to 4 | ||
writer.extend_from_slice(&[0u8; 16][..padding_needed]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need 16 bytes then? if max is 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it's a bit dangerous in a sense that PADDING_MULTIPLE
may change and this will throw a runtime error. why can't we do [0u8; padding_needed]
? because it's slower?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't we do
[0u8; padding_needed]
Because rust doesn't allow dynamically sized arrays on the stack right?
actually it's a bit dangerous in a sense that
PADDING_MULTIPLE
may change and this will throw a runtime error
This is a good point, if unlikely since the RFC for SCTP will probably never change in this regard. I can just use PADDING_MULTIPLE
as the size though. since padding_needed
is guaranteed to be < PADDING_MULTIPLE
As discussed in #360 the marshalling code has become a bottle neck in high bandwidth sending situations. I found two places that had a big effect on the performance, the hot path for this situation is marshalling packets with exactly one data chunk in them.
After this PR the marshalling is largely dominated by the CRC32 calculation which is... not easy to speed up.