Request for review (M): 7178363 G1: Remove the serial code for PrintGCDetails and make it a special case of the parallel code

[Bengt Rutisson]

Hi again,

Here is an updated webrev based on comments from John Cuthbertson:
http://cr.openjdk.java.net/~brutisso/7178363/webrev.02/

Here is a diff compared to the previous webrev:
http://cr.openjdk.java.net/~brutisso/7178363/webrev.01-02-diff/

Thanks,
Bengt

On 2012-07-20 14:17, Bengt Rutisson wrote:
>
> Hi all,
>
> Here is an updated webrev for this change:
> http://cr.openjdk.java.net/~brutisso/7178363/webrev.01/
>
> It turns out the the earlier change for 7178361 had introduced two 
> more issues: the heap transition information for the PrintGC output 
> and the output for evacuation failures had both been moved in an 
> unintended way. The above webrev corrects both of these chagne too. 
> Thanks John Cuthbertson for pointing me to the evacuation failure output.
>
> Bengt
>
>
> On 2012-07-19 11:01, Bengt Rutisson wrote:
>>
>> Hi again,
>>
>> Just in case anybody already started looking at this: I have updated 
>> the webrev since I had to make some changes to make it compile with 
>> the NMT fixes that have just made it into the hotspot-gc repository. 
>> Those updates were just to make it compile and not really related to 
>> my change, so I just overwrote the existing webrev. Just use the same 
>> link as before:
>>
>> http://cr.openjdk.java.net/~brutisso/7178363/webrev.00/
>>
>> Also, if you want to see what the new output looks like I am 
>> attaching a file called new.txt with an example from running 
>> SpecJBB2005 with this command line:
>>
>> -XX:+UseG1GC -XX:ParallelGCThreads=4 -XX:+UnlockExperimentalVMOptions 
>> -XX:G1LogLevel=finest -XX:+TraceGen0Time -Xms256m -Xmx2G
>>
>> I am also attaching a file called old.txt with what the output, using 
>> the same command line, looked like before my change. As you can see 
>> the differences are what I listed in my earlier email. You will also 
>> notice that the "old" version has an entry for the SATB filtering, 
>> even though all the entries are 0 (we didn't do a concurrent cycle so 
>> there has been no SATB filtering). This was a bug I just introduced 
>> with my last change (for 7178361), so the new example is more correct.
>>
>> Thanks,
>> Bengt
>>
>> On 2012-07-18 15:55, Bengt Rutisson wrote:
>>>
>>> Hi everyone,
>>>
>>> I would like some reviews of this change:
>>> http://cr.openjdk.java.net/~brutisso/7178363/webrev.00/
>>>
>>> This removes the special treatment for ParallelGCThreads=0 from the 
>>> G1 logging. I did keep the log output unchanged for that case. 
>>> Basically it just has one indentation level less and skips some 
>>> output. I am not sure this is really necessary since it is really a 
>>> special case. I'm open to change that special treatment too and just 
>>> have the same output as for ParallelGCThreads=1.
>>>
>>> The PrintGCDetails log output should be the same as before with 
>>> three minor adjustments:
>>>
>>> - The "Sum" is now not printed for the start and end values for GC 
>>> workers. This sum does not really make sense to me.
>>>
>>> - The "(ms)" unit was removed from output that aren't in 
>>> milliseconds (termination attempts for example).
>>>
>>> - The average value is now printed as a double for all types.
>>>
>>> I tried to clean up the code a bit and introduced a separate class, 
>>> Snippet WorkerDataArray, to keep track of the per thread logging. I 
>>> also introduced getters and setters to avoid having to make 
>>> G1CollectorPolicy and TraceGen0TimeData friend classes to 
>>> G1GCPhaseTimes.
>>>
>>> Thanks,
>>> Bengt
>>
>