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

John Cuthbertson john.cuthbertson at
Thu May 31 16:42:04 UTC 2012

Hi Bengt,

I think this looks really good. Great clean up.

One suggestion (but it's completely optional):

* Could you add a comment saying that TraceGen0Time actually collects 
data on _both_ young and mixed evacuation pauses (the latter may contain 
non-young regions - i.e. regions that are technically in Gen1) while 
TraceGen1Time collects data about full GCs?


On 05/30/12 05:32, Bengt Rutisson wrote:
> 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! :-)
> Bengt
>> 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