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

[Bengt Rutisson]

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.

Bengt

On 2012-05-28 22:37, Bengt Rutisson wrote:
>
> Hi all,
>
> Can I have a couple of code reviews for this change?
> http://cr.openjdk.java.net/~brutisso/7172279/webrev.01/
>
> 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