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

Mikael Vidstedt mikael.vidstedt at
Tue May 29 17:47:01 UTC 2012

On 2012-05-28 23:58, Bengt Rutisson wrote:
> 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.)

+1 for not changing the log output in a minor/update release. What does 
the "except" part mean?


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