RFR: 8259808: Add JFR event to detect GC locker stall [v4]

[Denghui Dong]

On Tue, 26 Jan 2021 09:42:59 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Denghui Dong has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   add GCLockerTracer::is_started() that makes the logic more clear
>
> src/hotspot/share/gc/shared/gcLocker.cpp line 101:
> 
>> 99:     verify_critical_count();
>> 100:     _needs_gc = true;
>> 101:     GCLockerTracer::start_gc_locker(_jni_lock_count);
> 
> Not really convinced that passing `_jni_lock_count` here gives a lot of information: this is the number of threads in a critical section at the point of the first thread needing a gc. It's probably better than nothing. At least this information should be added to the description of the event (if that is possible).

I think this field can be used to judge whether there are many threads that are often in a critical section, but I am not sure if it really helps to analyze the problem., and just as you said, it's better than nothing. An appropriate description of this field has been added.

> test/jdk/jdk/jfr/event/gc/detailed/TestGCLockerEvent.java line 116:
> 
>> 114:             ts[i] = new Thread(() -> {
>> 115:                 STALL_COUNT_SIGNAL.countDown();
>> 116:                 for (int j = 0; j < LOOP; j++) {
> 
> Since the test already uses WhiteBox, please use whitebox to trigger a gc instead of this dodgy method.

Triggering a GC is not enough, I hope these threads could be stall by the GC locker(call GCLocker::stall_until_clear) so that a correct assertion of the number of stall count could be added.
I think it could not be done by WhiteBox::youngGC/fullGC, please correct me if I'm wrong.

> src/hotspot/share/jfr/metadata/metadata.xml line 1095:
> 
>> 1093:   <Event name="GCLocker" category="Java Virtual Machine, GC, Detailed" description="GC Locker Information" label="GC Locker" startTime="true" thread="true" stackTrace="true">
>> 1094:     <Field type="uint" name="lockCount" label="Lock Count" />
>> 1095:     <Field type="uint" name="stallCount" label="Stall Count" />
> 
> Please add descriptions to the fields as mentioned above.

added

> test/jdk/jdk/jfr/event/gc/detailed/TestGCLockerEvent.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2021 Alibaba Group Holding Limited. All Rights Reserved.
> 
> Would it be possible to keep with the general format of copyright messages in other code, i.e. "Copyright (c) <year>, <copyright holder>. ..."? I.e. if possible please add a comma after the year.

Fixed.

> test/jdk/jdk/jfr/event/gc/detailed/TestGCLockerEvent.java line 34:
> 
>> 32: import sun.hotspot.WhiteBox;
>> 33: 
>> 34: /**
> 
> This block should be the first thing in the test after the copyright notice.

Fixed. But I notice that there are many other tests that didn't comply with this rule.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2088