<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: Times New Roman; font-size: 12pt; color: #000000'><br>----- bengt.rutisson@oracle.com wrote:
<br>>
<div>
<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">
<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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejmasa/8152208/webrev_delta.01_02/" target="_blank">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>Do you mean replace all the Matchers with the same OutputAnalyzer shouldMatch()? <br><br>As I did in the first delta?<br><br><pre><span class="removed">- String parallel_phase_leader = "Evacuate Collection Set: \\d+\\.\\d+ms";</span>
<span class="removed">- String std_out = output.getStdout();</span>
<span class="removed">- Matcher m = Pattern.compile(parallel_phase_leader, Pattern.MULTILINE).matcher(std_out);</span>
<span class="removed">- </span>
<span class="removed">- if (!m.find()) {</span>
<span class="removed">- throw new Exception("Could not find correct output for Evacuate Collection Set: in stdout," +</span>
<span class="removed">- " should match the pattern \"" + parallel_phase_leader + "\", but stdout is \n" + output.getStdout());</span>
<span class="removed">- } else {</span>
<span class="new">+ output.shouldMatch("Evacuate Collection Set: \\d+\\.\\d+ms");<br><br>and then again using the same OutputAnalyzer output?<br><br></span> String stats = "Ext Root Scanning \\(ms\\):";
Pattern stats_pattern = Pattern.compile(stats);
<span class="removed">- m.usePattern(stats_pattern);</span>
<span class="removed">- if (!m.find()) {</span>
<span class="removed">- throw new Exception("Could not find correct output for chosen statistics in stdout," +</span>
<span class="removed">- " should match the pattern \"" + stats + "\", but stdout is \n" + output.getStdout());</span>
<span class="removed">- } else {</span>
<span class="new">+ output.shouldMatch("Ext Root Scanning \\(ms\\):");</span><br></pre><br><br>>
<br>>
<blockquote cite="mid:56F2BFBB.5010902@oracle.com"><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>Doesn't output.getOutput() just return the entire buffer? I want to look at what's in the<br>log after my most recent match. For example when looking for the average <br><br><pre>[0.234s][debug ][gc,phases ] GC(0) Ext Root Scanning: Min: 0.6, Avg: 1.1, Max: 1.6, Diff: 1.0, Sum: 2.2<br><br>I first match "Ext Root Scanning:" and then want to match the next "Avg:".<br>That's what I think using my own Matcher allows me to do. shouldMatch()<br>tells me that there is a pattern somewhere in the output. <br><br>Thanks.<br><br>Jon<br></pre><br><br><br>>
<br>>
Thanks,<br>>
Bengt<br>>
<br>>
<blockquote cite="mid:56F2BFBB.5010902@oracle.com"><font face="Times New Roman, Times, serif"> <br>>
Full webrev is<br>>
<br>>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejmasa/8152208/webrev.02/" target="_blank">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">
<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">
<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">
<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 class="moz-txt-link-abbreviated" href="mailto:jon.masamitsu@oracle.com" target="_blank"></a><a class="moz-txt-link-abbreviated" href="mailto:jon.masamitsu@oracle.com" target="_blank">jon.masamitsu@oracle.com</a>></span>
wrote:<br>>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div> <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> <br>>
Hi Jon, <br>>
<br>>
On 2016-03-21 03:43, Jon Masamitsu wrote: <br>>
<blockquote>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 href="https://bugs.openjdk.java.net/browse/JDK-8152208" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8152208</a>
<br>>
<a 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 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 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" target="_blank">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">
<blockquote cite="mid:CANrGW1z1dZuvysUc-HarC7sYZkF+ExRWg4sPzqQMOENBoC4ZaA@mail.gmail.com">
<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> <br>>
Jon<br>>
<br>>
<blockquote> <br>>
Thanks, <br>>
Bengt <br>>
<br>>
<blockquote> <br>>
Thanks. <br>>
<br>>
Jon <br>>
</blockquote>
<br>>
</blockquote>
<br>>
</div>
</blockquote>
</div>
<br>>
</div>
</blockquote>
<br>>
</blockquote>
<br>>
</blockquote>
<br>>
</blockquote>
<br>>
</div>
<br>> </div></body></html>