Request for review (M): 7172279: G1: Clean up TraceGen0Time and TraceGen1Time data gathering

Bengt Rutisson bengt.rutisson at
Tue May 29 06:58:18 UTC 2012

Hi again,

I should also have mentioned that this change should not alter the log 
output. (Except for the ParallelGCThreads = 0 case which now will look 
the same as PrallelGCThreads >= 1 case.)

I tested this by leaving the old code in and comparing the output from 
the old code with the new code. In my tests they look exactly the same.


On 2012-05-28 22:37, Bengt Rutisson wrote:
> Hi all,
> Can I have a couple of code reviews for this change?
> Background
> I am preparing a change to remove the special treatment of the single 
> threaded case for G1 PrintGCDetails output. We should be able to use 
> the same code for both ParallelGCThreads = 0 and PrallelGCThreads >= 
> 1. This will hopefully simplify the logging code.
> In preparation for that change I would like to clean up the 
> TraceGen0Time and TraceGen1Time data gathering code a bit. It is 
> currently kind of difficult to follow the code and there is no clear 
> distinction between the data gathered for these two cases compared to 
> the data gathered for PrintGCDetails output.
> Here is what I tried to do:
> - Remove the unnecessary define_num_seq macro and the multiple 
> inheritance of the Summary class.
> - Replaced the above with two classes, TraceGen0TimeData and 
> TraceGen1TimeData,  that will encapsulate the data needed.
> - Made sure that the data is only updated if the corresponding flags 
> are turned on.
> - Removed the separate "non-parallel" case. All data is available even 
> with ParallelGCThreads = 0.
> - Removed the ExitAfterGCNum flag. I hardly think this is an 
> appropriate product flag to have in the code.
> - Removed some unused methods.
> - Removed 7 year old assert(true) with accompanying comments.
> - Removed the unused aux time concept.
> There was one thing that I was hesitant to remove, but finally decided 
> to remove anyway. It was the G1CollectorPolicy::check_other_times() 
> method. First I added the same method to TraceGen0TimeData. But it is 
> a lot of code and I am not sure it is very useful. It can't do any 
> exact checks and it seems unlikely to me that it will detect any 
> issues. Also, it seems to me like the code was in the wrong place. I 
> would have preferred it in some kind of verify method rather than in a 
> print method.
> Thanks,
> Bengt

More information about the hotspot-gc-dev mailing list