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

[Bengt Rutisson]

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:

http://cr.openjdk.java.net/~brutisso/7172279/webrev.02/

>
> 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?
>>> http://cr.openjdk.java.net/~brutisso/7172279/webrev.01/ 
>>> <http://cr.openjdk.java.net/%7Ebrutisso/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
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120530/fe6205b8/attachment.htm>