<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Thanks for the review Adam. I've corrected those style issues. <br>
</p>
<p>Now waiting on 2nd Reviewer.<br>
</p>
<pre class="moz-signature" cols="72">Regards,
Sean.</pre>
<div class="moz-cite-prefix">On 08/10/18 19:18, Adam Petcher wrote:<br>
</div>
<blockquote
cite="mid:08133352-029e-e2a1-b71d-ef681d8d787e@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<p>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. <br>
</p>
<p>Minor style issues(addressing them is optional):<br>
</p>
<p>Lines 848, 915, and 969 are longer than 80 characters<br>
Line 1075: no space before ? character<br>
</p>
<br>
<div class="moz-cite-prefix">On 10/2/2018 1:51 PM, Seán Coffey
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e83e9eed-1484-0036-a2d2-7a0c177c992f@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<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/%7Ecoffeys/webrev.8209862.v2/webrev/"
moz-do-not-send="true">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>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>