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

[Charlie Hunt]

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 ?

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

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/

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/20120529/12f946fc/attachment.htm>