RFR (M): 8074037: Refactor the G1GCPhaseTime logging to make it easier to add new phases

Kim Barrett kim.barrett at oracle.com
Fri Feb 27 23:43:29 UTC 2015


On Feb 27, 2015, at 10:38 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
> Can I have a couple of reviews for this cleanup change?
> 
> http://cr.openjdk.java.net/~brutisso/8074037/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8074037
> 
> […]
> The patch also introduces a new class called G1GCPhaseTimesTracker, which is a scoped object to simplify time measurements. I've also tried to move conversion of times down to when we print the log messages instead of when we store them. This removes several "* 1000.0" in the code. Eventually I would like to replace os::elapsedTime() with os::elapsed_counter(). But that should be done as a separate patch.

Before I proceed any further with this review, I feel like I'm getting
lost in unit changes. In some places values of msecs used to be used
but now we're using secs, right? But it seems like there might be some
confusion about that, and I'm not sure whether it's me or the code
that's confused.

For example:

src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp
1079       cost_per_card_ms = phase_times()->average_time(G1GCPhaseTimes::UpdateRS) / (double) _pending_cards;
1080       _cost_per_card_ms_seq->add(cost_per_card_ms);

I think the values that have been recorded in the phase_times are
secs, and average_time appears to just use the values as is.  So
shouldn't that now be cost_per_card_secs?  And the value added to
_cost_per_card_ms_seq needs to be cost_per_card_secs * 1000.0?

Oh how I wish for a units package!



More information about the hotspot-gc-dev mailing list