GCM Speed + Memory Mgmt. Improvements#783
Conversation
Encrypt latency: -3% to -23% (scales with payload size) Encrypt throughput: +3% to +31% (up to 2.87 GB/s at 8KB) Memory (bytes): -20% to -33% reduction Memory (allocations): -1 alloc (20% reduction: 5 to 4)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #783 +/- ##
==========================================
+ Coverage 81.63% 81.65% +0.02%
==========================================
Files 105 105
Lines 5837 5838 +1
==========================================
+ Hits 4765 4767 +2
+ Misses 688 687 -1
Partials 384 384
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:
|
| } | ||
| encryptedPayload := g.localGCM.Seal(nil, nonce, payload, additionalData) | ||
| r := make([]byte, len(raw)+len(nonce[4:])+len(encryptedPayload)) | ||
| finalSize := len(raw) + 8 + len(payload) + gcmTagLength |
There was a problem hiding this comment.
Hot dog nice change!
Would it be safer/easier to read to do make([]byte, 0, finalSize) and then do appends?
If we do get math wrong it won't crash? What's your take?
There was a problem hiding this comment.
Hey, it would be more readable to do what you propose e.g.
- r := make([]byte, finalSize)
- copy(r, raw)
- copy(r[len(raw):], nonce[4:])
-
- g.localGCM.Seal(r[len(raw)+8:len(raw)+8], nonce, payload, additionalData)
+ r := make([]byte, 0, finalSize)
+ r = append(r, raw...)
+ r = append(r, nonce[4:]...)
+ r = g.localGCM.Seal(r, nonce, payload, additionalData)
And yes, the current code will panic if we get the math wrong, your proposed change wouldn't, but all values are known at this point and the math is straightforward... so I wouldn't say its inherently safer.
I tested the change and it introduces a 4% regression at 8KB (I think because append is slower than copy for larger buffers). I'm leaning towards not making the change due to this performance hit, otherwise the readability benefit alone makes it worth it. What do you think?
There was a problem hiding this comment.
👍 100% agree.
It also is better probably to crash vs have unexpected behavior and who knows what the impact could be down the line.
I am super impressived/appreciative of these things. Keep doing great stuff inspiring me to get back into it :)
GCM Speed + Memory Mgmt. Improvements
Pretty significant results by doing some minor tweaks:
Pre-calculating the exact output size and having Seal() write encrypted data directly into the final buffer eliminates an intermediate allocation and copy operation.
Improvements Summary
Raw benchmark output before and after
Before
After