RFR (S): 8035398: Add card redirty time in "Other" time in G1

[Thomas Schatzl]

Hi Bengt,

  thanks for looking at this.

On Mon, 2014-03-03 at 20:57 +0100, Bengt Rutisson wrote:
> Hi Thomas,
> 
> On 2/25/14 2:52 PM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    can I have reviews for the following small change? It does nothing but
> > adding a "Redirty Cards" message showing the time the card redirtying
> > took as part of the "Other" time in the GC log.
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8035398
> >
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8035398/webrev/
> 
> This looks good. Thanks for adding this logging!
> 
> A couple of minor nits. Feel free to ignore them, I am fine with the 
> patch as it is.
> 
> g1CollectedHeap.cpp
> 
> 5291 
> g1_policy()->phase_times()->record_redirty_logged_cards_time_ms((os::elapsedTime() 
> - redirty_logged_cards_start) * 1000.0);
> 
> This is a very long line. I think I would prefer to store 
> "os::elapsedTime() - redirty_logged_cards_start) * 1000.0" in a local 
> variable to make it more readable.

Can I fix that in one go with some later commit? I intend to make
logging not use os::elapsedTime() any more too, but os::elapsedCounter()
or so like the other collectors.

Also, as mentioned in the other RFR about the logging for the evacuation
failure handling, this will be replaced by a scoped object.

> g1GCPhaseTimes.cpp
> 
>   317   if (G1DeferredRSUpdate) {
>   318     print_stats(2, "Redirty Cards", 
> _recorded_redirty_logged_cards_time_ms);
>   319   }
> 
> Most of the other "optional" values use a check like  if 
> (_recorded_redirty_logged_cards_time_ms > 0.0) instead of checking 
> flags. I am not sure what is better, I just thought it might be more 
> consistent to use that here too.

I actually prefer consistent output, it also simplifies testing for it.
If I added that, I would have to create a test that takes more than
0.0ms in this phase to check whether its there.

> TestGCLogMessages.java.
> Should we add a test without G1DeferredRSUpdate enabled?

G1DeferredRSUpdate is a debug flag, so you cannot (normally) change this
at runtime.

Thomas