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

Use speedy XOR from pion/transport #211

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Conversation

jech
Copy link
Member

@jech jech commented Jun 17, 2022

Now that we have a speedy version of xorBytes in pion/transport, we might as well use it. Benchmarks indicate an improvement of 10% to 20% in CTR encryption speed for large blocks, depending on the architecture.

The semantics of xor.XorBytes is a little bit different from the previous version — it panics if the destination is too short, while the previous version used to truncate the output. As far as I can tell, we never call it with a too small destination, we always use GrowBuffer beforehand. I've removed XorTestBufferSize, which was testing this behaviour; if any further tests are necessary, they should go into the xor package.

@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #211 (7a85ea5) into master (88859d2) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
- Coverage   76.54%   76.31%   -0.24%     
==========================================
  Files          17       17              
  Lines        1232     1220      -12     
==========================================
- Hits          943      931      -12     
  Misses        198      198              
  Partials       91       91              
Flag Coverage Δ
go 76.31% <100.00%> (-0.24%) ⬇️
wasm 75.81% <100.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crypto.go 91.30% <100.00%> (-2.99%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88859d2...7a85ea5. Read the comment docs.

go.mod Outdated
@@ -6,6 +6,6 @@ require (
github.com/pion/logging v0.2.2
github.com/pion/rtcp v1.2.9
github.com/pion/rtp v1.7.13
github.com/pion/transport v0.13.0
github.com/pion/transport v0.13.1-0.20220609023836-26aa3f6dca85
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is desirable. Do we need to wait for @Sean-Der to 'prod' pion/transport to v0.13.1 so we're referencing a release instead of a dated commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've now pushed the v0.13.1 tag to pion/transport. Can you try fixing up go.mod accordingly and we'll see if that works?

Now that we have a speedy version of xorBytes in pion/transport,
we might as well use it.  Benchmarks indicate an improvement
of 10% to 20% in CTR encryption speed for large blocks, depending
on the architecture.

Also remove TestXorBytesBuffer, which is no longer relevant.
@jech
Copy link
Member Author

jech commented Jun 22, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants