Deprecate XorBytes #315
Deprecate XorBytes #315
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #315 +/- ##
==========================================
- Coverage 83.03% 82.99% -0.04%
==========================================
Files 39 39
Lines 2729 2729
==========================================
- Hits 2266 2265 -1
- Misses 335 336 +1
Partials 128 128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Now that we no longer support Go 1.19, we might as well deprecate XorBytes and use the stdlib version instead.
jech
force-pushed
the
deprecate-xorbytes
branch
from
cc2a84f to
e62d789
Compare
|
@jech - it's a shame that the Go team weren't interested in picking up my PR for XorBytes. It just stalled after I completed the submission and as far as I can tell it was never reviewed. I would be happy to hear any suggestions you may have to get it picked up. Our application (which runs on 32-bit ARM, still very common for embedded systems) is connecting to IoT cameras, many of which are old and most of which use CTR. This PR would decrease performance in those cases and we would have to switch to a fork of pion with optimized XorBytes manually add back in. I'm generally against performance regressions so I don't think this PR should be merged. |
|
Please check the benchmarks, Adrian. The patch causes a 10% regression in WriteRTP. Assuming your code spends 40% of its CPU doing WriteRTP, this is a 4% regression to your code's performance, which should be negligible. Perhaps you could share some benchmarks of your application that show whether the change is significant? |
|
@jech - my code actually spends a lot less than 40% CPU on WriteRTP (this is used only for the camera talkback function), but it spends almost all of its CPU time on ReadRTP of which most of DecryptRTP. The WebRTC receiver is more or less a tight ReadRTP loop and on our platform (Allwinner H3) a single camera consumes 14.0% of one core in processing. We have a budget of 1.5 cores to support as many cameras as we can (the rest is needed for non-WebRTC processing functions), which means we support up to 10 cameras streaming in parallel, consuming a total of 1.4 cores. Without the ARM32 implemention of XorBytes, single camera CPU consumption increases to 15.2% (which matches your own benchmark closely enough), which means we need to drop the number of supported cameras from 10 to 9 to meet the total budget for WebRTC processing of 1.5 cores. In practice we wouldn't actually make things less capable in a product 'update' so we would need to maintain a separate pion fork with the XorBytes optimization back in. This is just extra work. In summary, this PR causes a performance regression in circumstances which can be meaningful for some applications (like ours) and doesn't add any benefit, so I don't see a reason to merge it. |
|
Thank you @jech! I should start a list of all the things that got added and we hope to remove someday. I completely forgot about this. @adriancable as long as you need this it will not be removed! I don't want to see you on a fork. |
[...]
These figures make sense. Adrian, may I encourage you to push https://go-review.googlesource.com/c/go/+/409394 again? It would be great if we could remove some code from Pion. |
|
@jech - yes, I will happily give it another go. I would love to see this merged into Go - that is where it really should live, rather than Pion. |
The Moving this code into the Go stdlib, will ensure that it gets reviewed. Failing that, removing it altogether is the safe way out. |
|
Hi @jech - the code in xor_asm.s (as something written specifically for Pion) has been reviewed in much the same way as all code written for the Pion code base has been. In general though, I would expect that Pion PRs get fewer eyes on them than Go PRs, which is somewhat inevitable given that we have a much smaller contributor base than Go has, but still want to maintain reasonable development velocity. Security issues are definitely a valid concern but I don't think apply more to this piece of code in Pion than other contributions, beyond the fact I suppose that being written in ARM32 assembly language makes it a little more 'esoteric' than most contributions. |
Now that we no longer support Go 1.19, we might as well deprecate
XorBytes, remove the ARM-32 assembler version, and use the stdlib version instead.This should not change anything on architectures other than 32-bit ARM. The only concern is that Go does not implement assembler code on 32-bit ARM (see https://go-review.googlesource.com/c/go/+/409394, which @adriancable never finished). The XorBytes benchmarks look pretty horrible (note that only the aligned versions are relevant to Pion):
However, 32-bit ARM does not implement hardware crypto, so once you factor in the cost of encryption, the difference is just 10% for CTR (which is seldom negotiated nowadays), and, as expected, no difference for GCM (which is used by modern browsers):