8214155: GCHeapLog inconsistents with CollectorCounters

[Yasumasa Suenaga]

Hi Thomas,

On 2018/11/22 23:34, Thomas Schatzl wrote:
> Hi Yasumasa,
> 
> On Thu, 2018-11-22 at 23:02 +0900, Yasumasa Suenaga wrote:
>> Hi Thomas,
>>
>> I erroneously assumed that `full` value in hs_err GC events means #
>> of STWs. I will withdraw this CR.

I've closes this issue as "Won't Fix".


> Maybe it is better to show this - that was mostly the question I had.
> :)
> 
>>> - the sun.gc.collector.2.name label "G1 stop-the-world phases"
>>> seems to be just wrong. Looking at
>>> src/jdk.jcmd/share/classes/sun/tools/jstat/resources/jstat_options,
>>> the comment for sun.gc.collector.2.* and the actual code indicates
>>> that this should actually be called something like "G1 concurrent
>>> (stop-the-world) phases".
>>
>> I agree with you.
>> Should I file this to JBS?
> 
> Please.

I filed:
   https://bugs.openjdk.java.net/browse/JDK-8214236


>>> - not sure it is useful to notify about remark and cleanup pause
>>> separately in the sun.gc.collector.2 statistics - isn't it
>>> sufficient to have a concurrent cycles count there? (idk about any
>>> dependencies).
>>
>> If sun.gc.collector.2 is not available, first STW phase will not be
>> counted when concurrent GC cycle is aborted: concurrent mode failure.
> 
> Currently the count will be updated in the Remark pause - wouldn't it
> be better to do that later then when the concurrent cycle really
> completed?

PerfCounter will be updated in VMOperation. In case of G1, it will be updated
at Remark and Cleanup.
   https://hg.openjdk.java.net/jdk/jdk/file/15fc92f4ae9a/src/hotspot/share/gc/g1/vm_operations_g1.cpp#l207

sun.gc.collector.2 shows STW phase of concurrent collector, and I think
it is useful because it will not be affected from concurrent mode failure.


>>> - some recent change might have introduced an issue: looking at
>>> g1ConcurrentMark.cpp:1359 where the "total_collection" count is
>>> increased there is some comment about races which I do not
>>> understand.
>>> However that count might be more appropriately increased in the
>>> Cleanup pause. Or not at all there, but rather at the end of the
>>> concurrent cycle (and isn't this a kind of "full" collection?).
>>>
>>> What would be the compatibility implications of all that?
>>
>> G1ConcurrentMark::cleanup() seems to be called at safepoint. It has
>> assertion for safepoint. So I guess it will not race.
>>
> 
> You are right, it's in the Cleanup pause so it should be fairly okay.
> Although there still could be an abort later. So it might be useful to
> move the increment of total_full_collections really into the
> (concurrent) end-of-concurrent cycle code?

If we do so, we might use atomic operation to update the counter.
I'm not sure ConcurrentGCThread will be suspended at safepoint.


Thanks,

Yasumasa


> Thanks,
>    Thomas
> 
>