RFR: 8209862:CipherCore performance improvement

[Adam Petcher]

The organization is better now, thanks. The code looks good to me, but I 
would like to request another review from Tony (or someone else who is 
familiar with this code) before you push. There is a lot of complexity 
here, and it is hard for me to be sure that everything will behave the 
same after the change. It will be helpful to have another person examine 
the new code to ensure that it is correct.

Minor style issues(addressing them is optional):

Lines 848, 915, and 969 are longer than 80 characters
Line 1075: no space before ? character


On 10/2/2018 1:51 PM, Seán Coffey wrote:
>
> Good points from Adam and Tony. I've taken them both on board. Sergey 
> has already eliminated a lot of duplicate code but there was further 
> optimizing possible in the two doFinal(..) methods. I've introduced a 
> new 'fillOutputBuffer' method to help. Please review :
>
> https://cr.openjdk.java.net/~coffeys/webrev.8209862.v2/webrev/
>
> regards,
> Sean.
>
>
> On 01/10/2018 15:32, Adam Petcher wrote:
>> 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.
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20181008/b4ff32ca/attachment.htm>