On Tue, 29 Sep 2020 20:22:55 GMT, Anthony Scarpino <ascarp...@openjdk.org>
wrote:
> 8253821: Improve ByteBuffer performance with GCM
Impressive update and performance improvement! I have no major concerns, all
comments are just about trivial details
like indents.
src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 660:
> 658: }
> 659:
> 660: /**
There is one extra indent..
src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 664:
> 662: * engineUpdate() and engineDoFinal().
> 663: */
> 664: private int bufferCrypt(ByteBuffer input, ByteBuffer output,
It looks like this method is copied from the CipherSpi. For maintenance, it
would be nice to add an extra comment to
state the copy and update. For example, "this method and implementation is
copied from javax.crypto.CipherSpi, with an
improvement for GCM mode."
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 947:
> 945: // create temporary output buffer if the estimated size is
> larger
> 946: // than the user-provided buffer.
> 947: if (output.length - outputOffset < estOutSize) {
"outputCapacity" could be used to replace "output.length - outputOffset", and
join the clause with the if-clause above.
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 928:
> 926: int outputCapacity = checkOutputCapacity(output, outputOffset,
> 927: estOutSize);
> 928: int offset = outputOffset; // 0 for decrypting
the line comment, "// 0 for decrypting", could be remove as the expression get
changed.
src/java.base/share/classes/com/sun/crypto/provider/CounterMode.java line 28:
> 26: package com.sun.crypto.provider;
> 27:
> 28: import java.nio.ByteBuffer;
There is no more update except this line. The new import ByteBuffer may not be
used.
src/java.base/share/classes/com/sun/crypto/provider/FeedbackCipher.java line
250:
> 248: * ByteBuffer methods should not be accessed as CipherCore and
> AESCipher
> 249: * copy the data to byte arrays. These methods are to satisfy the
> compiler.
> 250: *
there is an extra blank comment line.
src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 203:
> 201:
> 202: // Maximum buffer size rotating ByteBuffer->byte[] intrinsic copy
> 203: static final int MAX_LEN = 1024;
This filed could be private. I would like to declare class fields in the
beginning of a class, for easy
eyes-searching. Maybe, it is nice to use a multiple of the block size (for
example 64 * AES_BLOCK_SIZE), just in case
someone else update it to a weird value later.
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 144:
> 142:
> 143: // Maximum buffer size rotating ByteBuffer->byte[] intrinsic copy
> 144: final int MAX_LEN = 1024;
This filed could be private. I would like to declare class fields in the
beginning of a class, for easy eyes-searching.
Maybe, it is nice to use a multiple of the block size (for example 64 *
AES_BLOCK_SIZE), just in case someone else
update it to a weird value later.
-------------
Marked as reviewed by xuelei (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/411
