Looks like a nice improvement, but is it possible to do this without
duplicating code? For example, code almost identical to this also
appears starting on line 860:
971 } else { // encrypting
972 try {
973 outLen = finalNoPadding(finalBuf, finalOffset, output,
974 outputOffset, finalBufLen);
975 } finally {
976 // reset after doFinal() for GCM encryption
977 requireReinit = (cipherMode == GCM_MODE);
978 if (finalBuf != input) {
979 // done with internal finalBuf array. Copied to output
980 Arrays.fill(finalBuf, (byte) 0x00);
981 }
982 }
983 }
It may be possible to factor out the entire if/else statement and some
of the code around it into a separate method and do the short buffer
check and save/restore around it. Ideally, each doFinal method would
have only a small amount of code to either allocate the array or check
array lengths, and then they would call the same private method to do
most of the work.
I would suggest a noreg-sqe label on the ticket to document that
existing regression tests are used to ensure correctness of the modified
code.
Minor code style comments: check for long lines (e.g. line 856) and
missing spaces after commas in actual parameter lists (also e.g. line
856). Also, line 1076 is missing space around the '?' and ':'
characters. I can check the code again and give you the complete list
after we sort out the code duplication.
On 10/1/2018 9:11 AM, Seán Coffey wrote:
JDK-8207775 introduced some performance regressions in the ciphercore
area. Sergey Kuksenko has been looking at this and has contributed the
following patch:
http://cr.openjdk.java.net/~skuksenko/crypto/8209862/
bug report : https://bugs.openjdk.java.net/browse/JDK-8209862
I've been reviewing it and ran functionality and TCK testing. Didn't
see any issues. Sergey has also confirmed that the patch helps to
alleviate the performance issues introduced. More details regards
approach for fix are in the bug description.
Thanks Sergey! I'm looking for another review from security team.
