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

Bengt Rutisson bengt.rutisson at
Wed May 30 12:32:04 UTC 2012

Hi Charlie,

Thanks for looking at this!

On 2012-05-30 04:41, Charlie Hunt wrote:
> Nice cleanup Bengt.
> One observation which I found a bit confusing.
> In g1CollectorPolicy.cpp, line 2285 - 2287 we have the following code:
> _last_gc_was_young = gcs_are_young() ? true : false;
> _trace_gen0_time_data.increment_collection_count(!_last_gc_was_young);
> The !_last_gc_was_young threw me a bit since we were incrementing the 
> collection count on a "trace gen0".   Has there been consideration of 
> using an enumerated type for a young GC and mixed GC type rather than 
> using a boolean to represent the two?  Or, alternatively introducing a 
> mutator method for incrementing only _young_pause_num and a second 
> mutator method for incrementing _mixed_pause_num ?

Good point. I went with your last suggestion and added separate mutator 
methods for mixed and young collections. Here is an updated webrev:

> I didn't see anything incorrect in the implementation ... just found 
> this bit a little confusing.

Actually there was a mistake in the code too. I had moved the young and 
mixed counters from G1CollectorPolicy to TraceGen0TimeData. But since I 
wanted to verify my changes I had left the counters in G1CollectorPolicy 
and I forgot to remove them before the first webrev. So I removed them 
now too.

Thanks for drawing my attention to this as well! :-)

> hths,
> charlie ...
> On May 29, 2012, at 1:58 AM, 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.)
>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the hotspot-gc-dev mailing list