Request for Review - 8152208: Summary for phase times are incorrect with and without UseDynamicNumberOfGCThreads (original) (raw)

Bengt Rutisson bengt.rutisson at oracle.com
Thu Mar 24 08:15:46 UTC 2016

On 2016-03-24 08:27, Bengt Rutisson 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/webrevdelta.0102/ Sorry, I should have been more clear. I meant that you can replace all Matchers with shouldMatch(). Not just the first occurrence.

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.

After re-reading your comment I think I misunderstood. You meant that you need to get a particular group from the regexp? I think you can use the OutputAnalyzer.firstMatch() method for that:

http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/906fa01e86a0/test/testlibrary/jdk/test/lib/OutputAnalyzer.java#l327

Thanks, Bengt

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/ <http://cr.openjdk.java.net/%7Ejmasa/8152208/webrev.00/> Nice catch! Your change looks good. The method WorkerDataArray::sum(uint activethreads) 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/ <http://cr.openjdk.java.net/%7Ejmasa/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 parallelphaseleader = "Evacuate Collection Set: \d+\.\d+ms"; 71 String stdout = output.getStdout(); 72 Matcher m = Pattern.compile(parallelphaseleader, Pattern.MULTILINE).matcher(stdout); 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 "" + parallelphaseleader + "", 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/20160324/56dc808b/attachment.htm>

More information about the hotspot-gc-dev mailing list