xts: reduce tweak allocations #51
Conversation
|
This PR (HEAD: 4e46f9c) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/crypto/+/118535 to see it. Tip: You can toggle comments from me using the |
|
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
Message from Luke Champine: Patch Set 1: Hmm, I just noticed that the Cipher docstring says it "doesn't contain mutable state and therefore can be used concurrently." Does that make this change a non-starter? Or could the docstring be changed? Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
Message from Brad Fitzpatrick: Patch Set 1: Code-Review-1
Sorry, non-starter. We can't retroactively change the guarantees we once gave people. That's a compatibility change and violates https://golang.org/doc/go1compat. The x/* repos are under slightly looser rules, but we still are pretty strict about it. Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
Message from Luke Champine: Patch Set 1:
Wait, is the docstring even true? It claims that Cipher can be used concurrently, which to me means I can call Encrypt or Decrypt in multiple goroutines simultaneously -- but wouldn't that result in simultaneous calls to k2.Encrypt? In other words, Cipher can only be used concurrently if k2.Encrypt can be called concurrently. And the cipher.Block interface doesn't state that it must be safe for concurrent use. Granted, none of the cipher.Block implementations I tested resulted in race conditions, but something seems off here. Another option would be to protect tweak with a mutex. I'm not sure if it's worth it though; is repeated locking cheaper than repeated small allocations? A simple benchmark suggests that performance is nearly unchanged (<1% delta). So introducing a mutex removes the allocation without affecting performance. I can submit this version if desired. Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
Message from Brad Fitzpatrick: Patch Set 1: [+filippo, agl] for whether the concurrency comment is accurate or not. Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
Message from Filippo Valsorda: Patch Set 1: The docs should have the caveat "if the cipher.Block can be used concurrently". (Feel free to patch that in.) In practice most (all?) can on most (all?) architectures. It would be useful to map this out and document it. Filed golang/go#25882 It's unfortunate that any slice passed into cipher.Block must escape. I assume it's because it's an interface and we can do nothing about it? In this change you might want to use a sync.Pool, if you can show that it makes a difference in benchmarks. Do not introduce a hidden mutex, nobody would expect it. Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
This PR (HEAD: 3e18cd7) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/crypto/+/118535 to see it. Tip: You can toggle comments from me using the |
The call to k2.Encrypt causes tweak to escape to the heap, resulting in a 16-byte allocation for each call to Encrypt/Decrypt. Moving tweak into the Cipher struct would allow it to be reused, but this is ruled out by the Cipher docstring, which states that it is safe for concurrent use. Instead, manage tweak arrays with a sync.Pool. Benchmarks indicate that this amortizes allocation cost without impacting performance. benchmark old ns/op new ns/op delta BenchmarkXTS-4 234 245 +4.70% benchmark old allocs new allocs delta BenchmarkXTS-4 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkXTS-4 32 0 -100.00%
|
This PR (HEAD: 17c2532) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/crypto/+/118535 to see it. Tip: You can toggle comments from me using the |
|
Message from Luke Champine: Patch Set 3:
Thanks, I pushed a version with sync.Pool and the performance seems acceptable. benchcmp in the commit message. For future reference, can you elaborate more on why a mutex a bad idea? Is it because it prevents you from copying a Cipher value? Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
Message from Luke Champine: Patch Set 4: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
Message from Gerrit Bot: Uploaded patch set 5: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
lukechampine
changed the title
|
Message from Gerrit Bot: Uploaded patch set 6: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
Message from Gerrit Bot: Uploaded patch set 7: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/118535. |
|
This PR (HEAD: 14d81f5) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/crypto/+/118535 to see it. Tip: You can toggle comments from me using the |
The call to k2.Encrypt causes tweak to escape to the heap, resulting in a 16-byte allocation for each call to Encrypt/Decrypt. Moving tweak into the Cipher struct would allow it to be reused, but this is ruled out by the Cipher docstring, which states that it is safe for concurrent use. Instead, manage tweak arrays with a sync.Pool. Benchmarks indicate that this amortizes allocation cost without impacting performance. benchmark old ns/op new ns/op delta BenchmarkXTS-4 234 245 +4.70% benchmark old allocs new allocs delta BenchmarkXTS-4 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkXTS-4 32 0 -100.00% Change-Id: I5e0dd8c2e1a1078a151bbeb1d0760936b6b56216 GitHub-Last-Rev: 14d81f5 GitHub-Pull-Request: #51 Reviewed-on: https://go-review.googlesource.com/c/118535 Reviewed-by: Filippo Valsorda <filippo@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
The call to k2.Encrypt causes tweak to escape to the heap, resulting in a 16-byte allocation for each call to Encrypt/Decrypt. Moving tweak into the Cipher struct would allow it to be reused, but this is ruled out by the Cipher docstring, which states that it is safe for concurrent use. Instead, manage tweak arrays with a sync.Pool. Benchmarks indicate that this amortizes allocation cost without impacting performance. benchmark old ns/op new ns/op delta BenchmarkXTS-4 234 245 +4.70% benchmark old allocs new allocs delta BenchmarkXTS-4 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkXTS-4 32 0 -100.00% Change-Id: I5e0dd8c2e1a1078a151bbeb1d0760936b6b56216 GitHub-Last-Rev: 14d81f589f3ada2b19511d592000657af3410a51 GitHub-Pull-Request: golang/crypto#51 Reviewed-on: https://go-review.googlesource.com/c/118535 Reviewed-by: Filippo Valsorda <filippo@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
The call to k2.Encrypt causes tweak to escape to the heap, resulting in a 16-byte allocation for each call to Encrypt/Decrypt. Moving tweak into the Cipher struct would allow it to be reused, but this is ruled out by the Cipher docstring, which states that it is safe for concurrent use. Instead, manage tweak arrays with a sync.Pool. Benchmarks indicate that this amortizes allocation cost without impacting performance. benchmark old ns/op new ns/op delta BenchmarkXTS-4 234 245 +4.70% benchmark old allocs new allocs delta BenchmarkXTS-4 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkXTS-4 32 0 -100.00% Change-Id: I5e0dd8c2e1a1078a151bbeb1d0760936b6b56216 GitHub-Last-Rev: 14d81f589f3ada2b19511d592000657af3410a51 GitHub-Pull-Request: golang/crypto#51 Reviewed-on: https://go-review.googlesource.com/c/118535 Reviewed-by: Filippo Valsorda <filippo@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
The call to k2.Encrypt causes tweak to escape to the heap, resulting in a 16-byte allocation for each call to Encrypt/Decrypt. Moving tweak into the Cipher struct would allow it to be reused, but this is ruled out by the Cipher docstring, which states that it is safe for concurrent use. Instead, manage tweak arrays with a sync.Pool. Benchmarks indicate that this amortizes allocation cost without impacting performance. benchmark old ns/op new ns/op delta BenchmarkXTS-4 234 245 +4.70% benchmark old allocs new allocs delta BenchmarkXTS-4 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkXTS-4 32 0 -100.00% Change-Id: I5e0dd8c2e1a1078a151bbeb1d0760936b6b56216 GitHub-Last-Rev: 14d81f589f3ada2b19511d592000657af3410a51 GitHub-Pull-Request: golang/crypto#51 Reviewed-on: https://go-review.googlesource.com/c/118535 Reviewed-by: Filippo Valsorda <filippo@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
The call to k2.Encrypt causes tweak to escape to the heap, resulting in a 16-byte allocation for each call to Encrypt/Decrypt. Moving tweak into the Cipher struct would allow it to be reused, but this is ruled out by the Cipher docstring, which states that it is safe for concurrent use. Instead, manage tweak arrays with a sync.Pool. Benchmarks indicate that this amortizes allocation cost without impacting performance. benchmark old ns/op new ns/op delta BenchmarkXTS-4 234 245 +4.70% benchmark old allocs new allocs delta BenchmarkXTS-4 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkXTS-4 32 0 -100.00% Change-Id: I5e0dd8c2e1a1078a151bbeb1d0760936b6b56216 GitHub-Last-Rev: 14d81f589f3ada2b19511d592000657af3410a51 GitHub-Pull-Request: golang/crypto#51 Reviewed-on: https://go-review.googlesource.com/c/118535 Reviewed-by: Filippo Valsorda <filippo@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
The call to k2.Encrypt causes tweak to escape to the heap, resulting in a 16-byte allocation for each call to Encrypt/Decrypt. Moving tweak into the Cipher struct would allow it to be reused, but this is ruled out by the Cipher docstring, which states that it is safe for concurrent use. Instead, manage tweak arrays with a sync.Pool. Benchmarks indicate that this amortizes allocation cost without impacting performance. benchmark old ns/op new ns/op delta BenchmarkXTS-4 234 245 +4.70% benchmark old allocs new allocs delta BenchmarkXTS-4 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkXTS-4 32 0 -100.00% Change-Id: I5e0dd8c2e1a1078a151bbeb1d0760936b6b56216 GitHub-Last-Rev: 14d81f5 GitHub-Pull-Request: golang#51 Reviewed-on: https://go-review.googlesource.com/c/118535 Reviewed-by: Filippo Valsorda <filippo@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
The call to k2.Encrypt causes tweak to escape to the heap, resulting in a 16-byte allocation for each call to Encrypt/Decrypt. Moving tweak into the Cipher struct would allow it to be reused, but this is ruled out by the Cipher docstring, which states that it is safe for concurrent use. Instead, manage tweak arrays with a sync.Pool. Benchmarks indicate that this amortizes allocation cost without impacting performance. benchmark old ns/op new ns/op delta BenchmarkXTS-4 234 245 +4.70% benchmark old allocs new allocs delta BenchmarkXTS-4 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkXTS-4 32 0 -100.00% Change-Id: I5e0dd8c2e1a1078a151bbeb1d0760936b6b56216 GitHub-Last-Rev: 14d81f589f3ada2b19511d592000657af3410a51 GitHub-Pull-Request: golang/crypto#51 Reviewed-on: https://go-review.googlesource.com/c/118535 Reviewed-by: Filippo Valsorda <filippo@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
The call to k2.Encrypt causes tweak to escape to the heap, resulting
in a 16-byte allocation for each call to Encrypt/Decrypt. Moving
tweak into the Cipher struct would allow it to be reused, but this
is ruled out by the Cipher docstring, which states that it is safe
for concurrent use. Instead, manage tweak arrays with a sync.Pool.
Benchmarks indicate that this amortizes allocation cost without
impacting performance.
benchmark old ns/op new ns/op delta
BenchmarkXTS-4 234 245 +4.70%
benchmark old allocs new allocs delta
BenchmarkXTS-4 2 0 -100.00%
benchmark old bytes new bytes delta
BenchmarkXTS-4 32 0 -100.00%