RFR: 8209862:CipherCore performance improvement

[Seán Coffey]

Thanks for the review Adam. I've corrected those style issues.

Now waiting on 2nd Reviewer.

Regards,
Sean.

On 08/10/18 19:18, Adam Petcher wrote:
>
> 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/20181010/e87c886d/attachment.htm>