-
Notifications
You must be signed in to change notification settings - Fork 59
Reduce allocations in encrypt/decrypt RTP #355
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
Conversation
d611908 to
b22aa75
Compare
|
Beautiful, nice work @thesyncim :) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #355 +/- ##
==========================================
- Coverage 82.64% 82.63% -0.02%
==========================================
Files 19 19
Lines 1458 1457 -1
==========================================
- Hits 1205 1204 -1
Misses 140 140
Partials 113 113
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b22aa75 to
8f658b6
Compare
|
cc @atoppi |
JoTurk
left a comment
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.
Change is amazing thank you.
fb02431 to
ae33de0
Compare
ae33de0 to
57d6a38
Compare
srtp_cipher_aead_aes_gcm.go
Outdated
| useCryptex bool | ||
|
|
||
| // Pre-allocated buffer for IV to avoid heap allocation in hot path | ||
| rtpIV [12]byte |
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.
assuming this is safe, wouldn't it better to just change rtpInitializationVector and rtpInitializationVector to directly use this buffer instead of allocating their own?
BTW I'm not familiar with this code base, so totally ignore me if it doesn't make sense.
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.
@thesyncim Sorry i missed track of this PR.
what do you think about this refactor?
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.
Oh that's a great idea @asayyah!
It's a super simple change @thesyncim I think will make the code easier to follow. Change that function to generateRTPInitializationVector and then we can merge.
Sorry I haven't been quicker on this :( it's a great change to get rid of these allocations.
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.
If you don't have the time I can do it
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.
I can pick this up later today
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.
I did that. Could you check if everything looks good?
|
I also did the same optimization for the RTCP path. |
aa390fb to
66f2a85
Compare
JoTurk
left a comment
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.
looks good to me, thank you and sorry again that we lost track of this.
|
@atoppi Do you mind if you update this branch, a rebase or a merge commit will do it. I don't want to push to your branch in case you have local changes. |
Done |
|
@thesyncim @atoppi thank you so much for this. |
Uh oh!
There was an error while loading. Please reload this page.