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

[Bengt Rutisson]

On 2012-05-30 14:44, Charlie Hunt wrote:
> I like it. :-)

Thanks, Charlie!

Bengt

>
> charlie ...
>
> On May 30, 2012, at 7:32 AM, 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/70ff9eb4/attachment.htm>