Request for Review - 8152208: Summary for phase times are incorrect with and without UseDynamicNumberOfGCThreads

Tue Mar 29 21:05:53 UTC 2016

----- bengt.rutisson at oracle.com wrote: 
> 

> Hi Jon, 
> 
> 
> On 2016-03-23 17:09, Jon Masamitsu wrote: 
> 

Bengt, 
> 
> Thanks for the review. 
> 
> The delta webrev with you suggestion to use OutputAnalyzer shouldMatch is 
> here 
> 
> http://cr.openjdk.java.net/~jmasa/8152208/webrev_delta.01_02/ 
> 
> Sorry, I should have been more clear. I meant that you can replace all Matchers with shouldMatch(). Not just the first occurrence. 

Do you mean replace all the Matchers with the same OutputAnalyzer shouldMatch()? 

As I did in the first delta? 

-        String parallel_phase_leader = "Evacuate Collection Set: \\d+\\.\\d+ms"; -        String std_out = output.getStdout(); -        Matcher m = Pattern.compile(parallel_phase_leader, Pattern.MULTILINE).matcher(std_out); - -        if (!m.find()) { -          throw new Exception("Could not find correct output for Evacuate Collection Set: in stdout," + -            " should match the pattern \"" + parallel_phase_leader + "\", but stdout is \n" + output.getStdout()); -        } else { +        output.shouldMatch("Evacuate Collection Set: \\d+\\.\\d+ms"); 

and then again using the same OutputAnalyzer output? 

String stats = "Ext Root Scanning \\(ms\\):";
             Pattern stats_pattern = Pattern.compile(stats); -            m.usePattern(stats_pattern); -            if (!m.find()) { -              throw new Exception("Could not find correct output for chosen statistics in stdout," + -              " should match the pattern \"" + stats + "\", but stdout is \n" + output.getStdout()); -            } else { +        output.shouldMatch("Ext Root Scanning \\(ms\\):"); 

> 
> 

> I used my own Matcher in cases where I wanted to extract more information 
> after I had a match. 
> 
> I normally add a System.out.println(output.getOutput()); when I want more information about what is happening in the test. 

Doesn't output.getOutput() just return the entire buffer? I want to look at what's in the 
log after my most recent match. For example when looking for the average 

[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 

I first match "Ext Root Scanning:" and then want to match the next "Avg:". 
That's what I think using my own Matcher allows me to do. shouldMatch() 
tells me that there is a pattern somewhere in the output. 

Thanks. 

Jon 

> 
> Thanks, 
> Bengt 
> 
> 

> Full webrev is 
> 
> http://cr.openjdk.java.net/~jmasa/8152208/webrev.02/ 
> 
> Jon 
> 
> 
> On 03/22/2016 02:29 AM, Bengt Rutisson wrote: 
> 

> Hi Jon, 
> 
> 
> On 2016-03-22 00:05, Jon Masamitsu wrote: 
> 

Tao, 
> 
> I've updated the 01 version. The test has not changed. 
> Thanks again for catching that. 
> 
> Jon 
> 
> 
> On 03/21/2016 03:16 PM, Tao Mao wrote: 
> 

> This change has a bug: double counting get(0); should start with s = 0 

> 
Thanks. 
Tao Mao 
> 
> 
> On Mon, Mar 21, 2016 at 2:55 PM, Jon Masamitsu < jon.masamitsu at oracle.com > wrote: 
> 

Bengt, 
> 
> Thanks for the review. 
> 
> 
On 03/21/2016 02:13 AM, Bengt Rutisson wrote: 
> 

> Hi Jon, 
> 
> On 2016-03-21 03:43, Jon Masamitsu wrote: 
> 

The averages reported for phase times (for example "Ext Root Scanning") were 
> incorrect. Not all the per thread values were included in the sum and the 
> average value was incorrect (this with build 9-ea+1100) 
> 
> [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 
> [0.366s][trace][gc,phases,task ] GC(2) 0.4 0.3 
> 
> With the fix all values are included in the sum and the average is correct. 
> 
> [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 
> [2.830s][trace][gc,phases,task ] GC(0) 8.9 5.7 
> 
> https://bugs.openjdk.java.net/browse/JDK-8152208 
> http://cr.openjdk.java.net/~jmasa/8152208/webrev.00/ 
> 
> Nice catch! Your change looks good. 
> 
> The method WorkerDataArray<T>::sum(uint active_threads) just above the average() method has the same issue. Can you fix that too? 
> 
> Yes, indeed. 
> 
> I messed up the delta a bit so all the changes are in the workerDataArray.inline.hpp 
> webrev. The test has not changed. 
> 
> http://cr.openjdk.java.net/~jmasa/8152208/webrev.01/ 
> 
> The code changes look good. (I realize that the sum() iteration was actually correct, but I your changes are much easier to read.) 
> 
> The test could be simplified a bit if you use the shouldMatch() method on the OutputAnalyzer rather than compiling your own regexps. 
> 
> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/9037ef388634/test/testlibrary/jdk/test/lib/OutputAnalyzer.java#l224 
> 
> You already have an instance of OutputAnalyzer in your test: 
> 
> 
> 64 OutputAnalyzer output = new OutputAnalyzer(pb.start()); 
> 
> So, this: 
> 
> 
> 70 String parallel_phase_leader = "Evacuate Collection Set: \\d+\\.\\d+ms"; 
> 71 String std_out = output.getStdout(); 
> 72 Matcher m = Pattern.compile(parallel_phase_leader, Pattern.MULTILINE).matcher(std_out); 
> 73 
> 74 if (!m.find()) { 
> 75 throw new Exception("Could not find correct output for Evacuate Collection Set: in stdout," + 
> 76 " should match the pattern \"" + parallel_phase_leader + "\", but stdout is \n" + output.getStdout()); 
> 77 } else { 
> 
> Could be replaced by the single line: 
> 
> output.shouldMatch("Evacuate Collection Set: \\d+\\.\\d+ms"); 
> 
> Similarly for the other matching in the test. 
> 
> Thanks, 
> Bengt 
> 
> 
> 

> 
> 

> Jon 
> 
> 

> Thanks, 
> Bengt 
> 
> 

> Thanks. 
> 
> Jon 
> 
> 
> 
> 
> 
> 
> 
> 
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160329/1270e1b7/attachment.htm>