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

[Bengt Rutisson]

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

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<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/56dc808b/attachment.htm>