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

[Charlie Hunt]

I like it. :-)

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/20120530/3d7e369d/attachment.htm>