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

[Jon Masamitsu]

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/

I used my own Matcher in cases where I wanted to extract more information
after I had a match.

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 <mailto: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<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/
>>>     <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 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/20160323/55fd3298/attachment.htm>