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
x/crypto/openpgp: streaming encryption allocates excessively #26578
Comments
It doesn't look like cipher.StreamWriter is used anywhere else in x/crypto so if this code is added, it seems okay to not (yet) worry about sharing it. @twmb it's not super important since the benefit is obvious but figured I'd ask: is that benchmark you're using open source? (Might want to include some benchmark if you end up sending a proper CL.) /cc @FiloSottile |
Writing this benchmark was pretty tricky since apparently the input buffer size and the character set in that buffer matters a lot. The allocations surprisingly drop significantly when the input is binary data or when the input is <=32KiB. When the input is small enough, the gzip wrapped case doesn't even allocate, which is pretty surprising to me since this still goes through the same allocating path. I'm not sure what's up there. Edit: also, changing the code that inspired this idea to use small writes doesn't drop the allocations, so I think something with escape analysis and smaller writes may be going on in the benchmark below (hard to tell). Anyway, a recreating benchmark:
|
Closing due to prior discussion of openpgp being removed from x/crypto. |
x/crypto/openpgp.Encrypt
wraps the to-be-encryptedio.Writer
with apacket.seMDCWriter
. Thisio.WriteCloser
encrypts data on every write using acipher.StreamWriter
.Every write to a
cipher.StreamWriter
allocates a slice as long as the input write. This is fine if we are allocating a bulk of data at once, but if we are streaming encryption over time, these allocations are unnecessary. Worse, if we are streaming through a different writer (i.e., if we stream through gzip compression), that writer may cut the writes even smaller.We cannot add a reusable buffer to
cipher.StreamWriter
because all of its fields are exported. Becausecipher.StreamWriter
contains nearly no code in its methods, it is small enough to copy the struct and its methods into thepacket
package and add a reusable buffer to it. This cuts allocations significantly for streaming input and is basically a no-op for bulk encryption.The following diff would accomplish what I am describing:
If this is an OK idea, I would like to issue this patch. For some numbers, applying this patch to a project results in the following bench diff (names changed):
The text was updated successfully, but these errors were encountered: