<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>The updated webrev looks good. I will proceed with
pre-integration testing and push the changes on your behalf once
the testing is done.</p>
<p>In the mean time, if you have any particular wording for the
commit message, please send it my way.</p>
Thanks,<br>
Valerie<br>
<div class="moz-cite-prefix">On 4/3/2020 5:40 AM, Alexey Bakhtin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:F701FE92-49A1-468A-82DE-E014DA522BB1@azul.com">
Hello Valeri,
<div class=""><br class="">
</div>
<div class="">Thank you for review. Please, have a look at updated
patch at : <a class="moz-txt-link-freetext"
href="https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v1/"
moz-do-not-send="true">https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v1/</a></div>
<div class=""><br class="">
</div>
<div class="">I’ve fixed your findings and renamed test class to
ThreadSafetyTest. Jtreg already has <span class="">ThreadSafeTest
class for </span><span class="">java.text.Normalizer
functionality.</span></div>
<div class=""><span class=""><br class="">
</span></div>
<div class="">Thank you</div>
<div class="">Alexey</div>
<div class=""><br class="">
<div>
<blockquote type="cite" class="">
<div class="">On 3 Apr 2020, at 02:51, Valerie Peng <<a
href="mailto:valerie.peng@oracle.com" class=""
moz-do-not-send="true">valerie.peng@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">
<p class="">Hi Alexey,</p>
<p class="">In general looks pretty good, just some
comments on the regression test:</p>
<p class="">- line 28: The duration value 10 may be
lowered to shorten the execution time. On a Linux
machine, with threadFactor=5, each digest algo takes
about (2xduration+2) sec and overall takes ~283sec.
Cutting the duration value drastically to 2, I still
can reproduce the bug. Maybe 4 would be a better
default value considering the execution time.
<br class="">
</p>
<p class="">- line 43: add "SHA-512/224", "SHA-512/256"
to the ALGORITHM_ARRAY?<br class="">
</p>
<p class="">- line 50: instead of hardcoding 5 here, why
not just use threadsFactor (assigned with default
value 5 on line 48)?</p>
<p class="">- line 52-55: I think you meant to use 5
(instead of the value 180) as the default min duration
on line 52. Then, use duration variable instead of the
hardcoded 5 on line54. For testing purpose, why not
just use the supplied value? You already have default
values if none are supplied.</p>
<p class="">- Consider iterating through existing
providers and remove isSHA3supported() method. This
way all providers which supports the digest algorithm
are tested and no provider-specific knowledge is
needed. For example:</p>
<pre class=""> Provider[] provs = Security.getProviders();</pre>
<pre class=""> for (Provider p : provs) {</pre>
<pre class=""> System.out.println("Testing provider " + p.getName() + "...");</pre>
<pre class=""> for (String alg: ALGORITHM_ARRAY) {</pre>
<pre class=""> try {</pre>
<pre class=""> MessageDigest md = MessageDigest.getInstance(alg, p);</pre>
<pre class=""> testThreadSafe(md, input, nTasks, duration, false);</pre>
<pre class=""> .... </pre>
<p class="">- line 76: missing space char between } and
catch.</p>
Thanks,<br class="">
Valerie<br class="">
<div class="moz-cite-prefix"><br class="">
</div>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
</body>
</html>