-
Notifications
You must be signed in to change notification settings - Fork 56
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 crypto/subtle.XORBytes #294
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #294 +/- ##
==========================================
- Coverage 83.30% 83.11% -0.20%
==========================================
Files 40 39 -1
Lines 2761 2724 -37
==========================================
- Hits 2300 2264 -36
+ Misses 337 335 -2
- Partials 124 125 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6840abe
to
1b5363e
Compare
Looks good to me :) I think we are ready to bump the minimum Go version to 1.20 by now. Can you add a nolint-Comment to silence the linter error? |
Please don't, Debian stable still uses 1.19.
Done. |
Just fixed a bug in the xor_old code, which wasn't optimising the AMD64 case correctly. About to push. |
The function XORBytes has been included in the Go standard library since Go version 1.20. This allows us to remove a bunch of assembler code, and instead rely on the stdlib. There are only three versions remaining: - xor_generic, which simply calls the stdlib function; - xor_old, which uses unsafe Go and is used with Go 1.19; - xor_arm, which is written in assembly and is used on 32-bit ARM, since the stdlib doesn't implement a fast version of XORBytes on that architecture.
Okay, we just a flake WASM test failing before. I've reran the pipeline. It passes now. Feel free to merge it :) |
I don't have commit rights (I'd need to setup 2FA on github, which I'm not willing to do). |
The function XORBytes has been included in the Go standard library since Go version 1.20. This allows us to remove a bunch of assembler code, and instead rely on the stdlib.
There are only three versions remaining:
At least on AMD64 using Go 1.22, performance is unchanged:
Of course, there is a significant regression under Go 1.19 and earlier. Note that only the aligned case is relevant for Pion (see
xorBytesCTR
insrtp/crypto.go
).However, Go 1.19. is obsolescent (it's mostly relevant for Debian Stable), so it's worth to pay that price in order to remove 350 lines of inscrutable assembler: