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

[Bengt Rutisson]

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.

>
> 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.

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<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/20160324/8d99b461/attachment.htm>