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

[Mikael Vidstedt]

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?

Thanks,
Mikael

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