On Sun, 13 Nov 2022 02:54:10 GMT, Anthony Scarpino <ascarp...@openjdk.org>
wrote:
> I would like a review of an update to the GCM code. A recent report showed
> that GCM memory usage for TLS was very large. This was a result of in-place
> buffers, which TLS uses, and how the code handled the combined intrinsic
> method during decryption. A temporary buffer was used because the combined
> intrinsic does gctr before ghash which results in a bad tag. The fix is to
> not use the combined intrinsic during in-place decryption and depend on the
> individual GHASH and CounterMode intrinsics. Direct ByteBuffers are not
> affected as they are not used by the intrinsics directly.
>
> The reduction in the memory usage boosted performance back to where it was
> before despite using slower intrinsics (gctr & ghash individually). The
> extra memory allocation for the temporary buffer out-weighted the faster
> intrinsic.
>
>
> JDK 17: 122913.554 ops/sec
> JDK 19: 94885.008 ops/sec
> Post fix: 122735.804 ops/sec
>
> There is no regression test because this is a memory change and test coverage
> already existing.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
580:
> 578: * an upper limit on the number of blocks encrypted in the intrinsic.
> 579: *
> 580: * For decrypting in-place byte[], calling methods must ct must set
> to null
end of sentence mangled
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
592:
> 590:
> 591: int len = 0;
> 592: // Loop if input length is greater than the SPLIT_LEN
comment doesn't add anything not already obvious from the code
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
596:
> 594: int partlen;
> 595: while (inLen >= SPLIT_LEN) {
> 596: partlen = implGCMCrypt0(in, inOfs + len, SPLIT_LEN, ct,
why not `int partlen` and get rid of line 594
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
603:
> 601: }
> 602:
> 603: // Finish any remaining data
comment doesn't add anything special
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
694:
> 692: int originalOutOfs = 0;
> 693:
> 694: // True if op is in-place array decryption with the input &
> output
// Setting `inPlaceArray` to true turns off combined intrinsic processing.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
764:
> 762: byte[] array;
> 763: if (encryption) {
> 764: array = dst.array();
You could factor out lines 764 and 770 by changing line 762 to
`byte[] array = encryption ? dst.array() : src.array();`
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1644:
> 1642: // Clear output data
> 1643: dst.reset();
> 1644: // If this is an in-place array, don't zero the src
The comment doesn't jive with the line of code on the next line. It is the
inverse of the comment.
-------------
PR: https://git.openjdk.org/jdk/pull/11121
