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

[Bengt Rutisson]

Hi Thomas,

On 3/3/14 9:07 PM, Thomas Schatzl wrote:
> 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.

Sure, that is fine.

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

Sounds like a good plan! :)

>
>> 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.

Fair enough. I'm fine with that.

>
>> TestGCLogMessages.java.
>> Should we add a test without G1DeferredRSUpdate enabled?
> G1DeferredRSUpdate is a debug flag, so you cannot (normally) change this
> at runtime.

Ah. Right. That's good then.

Ship it!
Bengt

>
> Thomas
>