RFR: 8209862:CipherCore performance improvement

[Seán Coffey]

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/20181002/e2f51413/attachment.htm>