RFR (XS): 8200730: Fix debug=gc+phases time tracking in Remark and Cleanup

[sangheon.kim]

Hi Thomas,

On 04/16/2018 07:11 AM, Thomas Schatzl wrote:
> Hi Stefan,
>
> On Mon, 2018-04-16 at 14:09 +0200, Stefan Johansson wrote:
>> Hi Thomas,
>>
>> On 2018-04-16 13:14, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     ping for any reviewer...
>>>
>>> Thanks,
>>>     Thomas
>>>
>>> On Wed, 2018-04-04 at 14:26 +0200, Thomas Schatzl wrote:
>>>> Hi all,
>>>>
>>>>     can I have reviews to let the recently introduced timing
>>>> measurements in Remark and Cleanup pauses actually show useful
>>>> numbers?
>>>>
>>>> The problem is that the GCTraceTime instances were not assigned
>>>> to a variable inside a scope so the compiler immediately
>>>> destructed it, always giving "0.000ms" lengths.
>>>>
>>>> Also added to use the gc timer to these lines for JFR support.
>>>>
>>>> I would like to think this is a trivial change.
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8200730
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8200730/webrev
>> Sorry for missing this one :)
>>
>> Thanks for fixing this. Two things:
>> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
>> 1994     GCTraceTime(Debug, gc)("Clear Next Bitmap");
>>
>> This line should to be changed as well.
> That's already been wrong for a long time -__-
>
>> ---
>>
>> The second thing is a partly pre-existing problem and I suggest you
>> fix all occurrences in this file. The name "trace" is a bit
>> misleading when the debug-level is "Debug", which is the case for all
>> GCTraceTime in this file. I would prefer using "debug", like on this
>> line:
>> 1654     GCTraceTime(Debug, gc, phases) debug("Weak Processing",
>> _gc_timer_cm);
>> -----
> Fixed too.
>
> http://cr.openjdk.java.net/~tschatzl/8200730/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8200730/webrev.1/ (full)
Webrev.1 looks good.

Thanks,
Sangheon


>
> Thanks for your review.
>
> Thomas
>