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

[John Cuthbertson]

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?

JohnC

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:
>
> 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/20120531/89b1efc3/attachment.htm>