On Thu, 13 Jun 2024 20:25:22 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix clone(), accept review suggestions. > > src/java.base/share/classes/sun/security/provider/SHA3.java line 73: > >> 71: // The following array is allocated to size WIDTH bytes, but we only >> 72: // ever use the first blockSize bytes it (for bytes <-> long >> conversions) >> 73: private byte[] byteState = new byte[WIDTH]; > > Since we are storing the state in longs now, this "byte <-> long" conversion > can be made through a local variable, right? Is there a reason for having > this `byteState` field with size WIDTH bytes? This is interesting: if I use WIDTH (or in blockSize) long arrays in the local level, the performance drops a few per cents. Even more when I only declare the local array in the block in which it is used. However, since we really only need 8 bytes, if I allocate that at the beginning of the function, I don't see that performance drop. So I rewrote the output loop in the function and got rid of the class level declaration. > src/java.base/share/classes/sun/security/provider/SHA3.java line 121: > >> 119: } >> 120: implCompress(buffer, 0); >> 121: int availableBytes = buffer.length; > > change to `blockSize` as in `implCompress0(...)`? Yes, thanks, I wanted to, just forgot. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19632#discussion_r1639572500 PR Review Comment: https://git.openjdk.org/jdk/pull/19632#discussion_r1639572413
