RFR: 8209862:CipherCore performance improvement

[Anthony Scarpino]

I'm ok with the changes.

Tony

On 10/10/2018 09:26 AM, Seán Coffey wrote:
> 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.
>>>>>
>>>>
>>>
>>
>