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

[Bengt Rutisson]

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/20160322/cc5eb51e/attachment.htm>