<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>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 : <br>
</p>
<p><a class="moz-txt-link-freetext" href="https://cr.openjdk.java.net/~coffeys/webrev.8209862.v2/webrev/">https://cr.openjdk.java.net/~coffeys/webrev.8209862.v2/webrev/</a></p>
<p>regards,<br>
Sean.<br>
</p>
<br>
<div class="moz-cite-prefix">On 01/10/2018 15:32, Adam Petcher
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:4f202b2f-9d83-c043-1f29-a53e174aa07c@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
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:<br>
<br>
<pre><span class="changed"> 971 } else { // encrypting</span>
<span class="changed"> 972 try {</span>
<span class="changed"> 973 outLen = finalNoPadding(finalBuf, finalOffset, output,</span>
<span class="changed"> 974 outputOffset, finalBufLen);</span>
<span class="changed"> 975 } finally {</span>
<span class="changed"> 976 // reset after doFinal() for GCM encryption</span>
<span class="changed"> 977 requireReinit = (cipherMode == GCM_MODE);</span>
<span class="changed"> 978 if (finalBuf != input) {</span>
<span class="changed"> 979 // done with internal finalBuf array. Copied to output</span>
<span class="changed"> 980 Arrays.fill(finalBuf, (byte) 0x00);</span>
<span class="changed"> 981 }</span>
<span class="changed"> 982 }</span>
<span class="changed"> 983 }
</span><span class="changed"></span></pre>
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.<br>
<br>
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.<br>
<br>
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. <br>
<br>
<br>
<pre><span class="changed"></span></pre>
<div class="moz-cite-prefix">On 10/1/2018 9:11 AM, Seán Coffey
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:778f299c-c2b0-be47-59a9-3690ed3f9b40@oracle.com">JDK-8207775
introduced some performance regressions in the ciphercore area.
Sergey Kuksenko has been looking at this and has contributed the
following patch: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eskuksenko/crypto/8209862/"
moz-do-not-send="true">http://cr.openjdk.java.net/~skuksenko/crypto/8209862/</a>
<br>
bug report : <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8209862"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8209862</a>
<br>
<br>
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. <br>
<br>
Thanks Sergey! I'm looking for another review from security
team. <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>