<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
Hi Jon,<br>
<br>
<div class="moz-cite-prefix">On 2016-03-23 17:09, Jon Masamitsu
wrote:<br>
</div>
<blockquote cite="mid:56F2BFBB.5010902@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<font face="Times New Roman, Times, serif">Bengt,<br>
<br>
Thanks for the review.<br>
<br>
The delta webrev with you suggestion to use OutputAnalyzer
shouldMatch is<br>
here<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/8152208/webrev_delta.01_02/">http://cr.openjdk.java.net/~jmasa/8152208/webrev_delta.01_02/</a><br>
</font></blockquote>
<br>
Sorry, I should have been more clear. I meant that you can replace
all Matchers with shouldMatch(). Not just the first occurrence. <br>
<br>
<blockquote cite="mid:56F2BFBB.5010902@oracle.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
I used my own Matcher in cases where I wanted to extract more
information<br>
after I had a match.<br>
</font></blockquote>
<br>
I normally add a System.out.println(output.getOutput()); when I want
more information about what is happening in the test.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:56F2BFBB.5010902@oracle.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
Full webrev is<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/8152208/webrev.02/">http://cr.openjdk.java.net/~jmasa/8152208/webrev.02/</a><br>
<br>
Jon<br>
</font><br>
<div class="moz-cite-prefix">On 03/22/2016 02:29 AM, Bengt
Rutisson wrote:<br>
</div>
<blockquote cite="mid:56F11061.5030605@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
Hi Jon,<br>
<br>
<div class="moz-cite-prefix">On 2016-03-22 00:05, Jon Masamitsu
wrote:<br>
</div>
<blockquote cite="mid:56F07E31.2000601@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<font face="Times New Roman, Times, serif">Tao,<br>
<br>
I've updated the 01 version. The test has not changed.<br>
Thanks again for catching that.<br>
<br>
Jon<br>
</font><br>
<div class="moz-cite-prefix">On 03/21/2016 03:16 PM, Tao Mao
wrote:<br>
</div>
<blockquote
cite="mid:CANrGW1z1dZuvysUc-HarC7sYZkF+ExRWg4sPzqQMOENBoC4ZaA@mail.gmail.com"
type="cite">
<div dir="ltr">This change has a bug: double counting
get(0); should start with s = 0
<div><br>
</div>
<div>Thanks.</div>
<div>Tao Mao</div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Mar 21, 2016 at 2:55 PM,
Jon Masamitsu <span dir="ltr"><<a
moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:jon.masamitsu@oracle.com"><a class="moz-txt-link-abbreviated" href="mailto:jon.masamitsu@oracle.com">jon.masamitsu@oracle.com</a></a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <font
face="Times New Roman, Times, serif">Bengt,<br>
<br>
Thanks for the review.<br>
</font><span class=""><br>
<div>On 03/21/2016 02:13 AM, Bengt Rutisson wrote:<br>
</div>
<blockquote type="cite"> <br>
Hi Jon, <br>
<br>
On 2016-03-21 03:43, Jon Masamitsu wrote: <br>
<blockquote type="cite">The averages reported
for phase times (for example "Ext Root
Scanning") were <br>
incorrect. Not all the per thread values were
included in the sum and the <br>
average value was incorrect (this with build
9-ea+1100) <br>
<br>
[0.366s][debug][gc,phases ]
GC(2) Ext Root Scanning (ms): Min: 0.3,
Avg: 0.2, Max: 0.4, Diff: 0.0, Sum: 0.3 <br>
[0.366s][trace][gc,phases,task ]
GC(2) 0.4 0.3
<br>
<br>
With the fix all values are included in the
sum and the average is correct. <br>
<br>
[2.830s][debug][gc,phases ]
GC(0) Ext Root Scanning (ms): Min: 5.7,
Avg: 7.3, Max: 8.9, Diff: 3.1, Sum: 14.6 <br>
[2.830s][trace][gc,phases,task ]
GC(0) 8.9 5.7
<br>
<br>
<a moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8152208"
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8152208</a>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejmasa/8152208/webrev.00/"
target="_blank">http://cr.openjdk.java.net/~jmasa/8152208/webrev.00/</a>
<br>
</blockquote>
<br>
Nice catch! Your change looks good. <br>
<br>
The method WorkerDataArray<T>::sum(uint
active_threads) just above the average() method
has the same issue. Can you fix that too? <br>
</blockquote>
<br>
</span> Yes, indeed.<br>
<br>
I messed up the delta a bit so all the changes are
in the workerDataArray.inline.hpp<br>
webrev. The test has not changed.<br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejmasa/8152208/webrev.01/"
target="_blank">http://cr.openjdk.java.net/~jmasa/8152208/webrev.01/</a><br>
</div>
</blockquote>
</div>
</div>
</blockquote>
</blockquote>
<br>
The code changes look good. (I realize that the sum() iteration
was actually correct, but I your changes are much easier to
read.)<br>
<br>
The test could be simplified a bit if you use the shouldMatch()
method on the OutputAnalyzer rather than compiling your own
regexps.<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/9037ef388634/test/testlibrary/jdk/test/lib/OutputAnalyzer.java#l224">http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/9037ef388634/test/testlibrary/jdk/test/lib/OutputAnalyzer.java#l224</a>
<br>
<br>
You already have an instance of OutputAnalyzer in your test:<br>
<br>
<br>
64 OutputAnalyzer output = new
OutputAnalyzer(pb.start());<br>
<br>
So, this:<br>
<br>
<br>
70 String parallel_phase_leader = "Evacuate Collection
Set: \\d+\\.\\d+ms";<br>
71 String std_out = output.getStdout();<br>
72 Matcher m = Pattern.compile(parallel_phase_leader,
Pattern.MULTILINE).matcher(std_out);<br>
73 <br>
74 if (!m.find()) {<br>
75 throw new Exception("Could not find correct
output for Evacuate Collection Set: in stdout," +<br>
76 " should match the pattern \"" +
parallel_phase_leader + "\", but stdout is \n" +
output.getStdout());<br>
77 } else {<br>
<br>
Could be replaced by the single line:<br>
<br>
output.shouldMatch("Evacuate Collection Set: \\d+\\.\\d+ms");<br>
<br>
Similarly for the other matching in the test.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<blockquote cite="mid:56F07E31.2000601@oracle.com" type="cite">
<blockquote
cite="mid:CANrGW1z1dZuvysUc-HarC7sYZkF+ExRWg4sPzqQMOENBoC4ZaA@mail.gmail.com"
type="cite">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
Jon<br>
<br>
<blockquote type="cite"> <br>
Thanks, <br>
Bengt <br>
<br>
<blockquote type="cite"> <br>
Thanks. <br>
<br>
Jon <br>
</blockquote>
<br>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>