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

[Denghui Dong]

On Fri, 22 Jan 2021 09:17:45 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Denghui Dong has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Refactor based on comments
>
> src/hotspot/share/gc/shared/gcTraceSend.cpp line 354:
> 
>> 352: 
>> 353: void GCLockerTracer::start_gc_locker(const jint jni_lock_count) {
>> 354:   assert(SafepointSynchronize::is_at_safepoint(), "sanity");
> 
> Maybe add assertions that `_jni_lock_count` and `_stall_count` is 0 to ensure this is not called multiple times for a single event.

Makes sense.
Added.

> src/hotspot/share/gc/shared/gcTraceSend.cpp line 369:
> 
>> 367: void GCLockerTracer::report_gc_locker() {
>> 368:   Ticks zero;
>> 369:   if (_needs_gc_start_timestamp != zero) {
> 
> Why is this needed?

Because we can't assume that EventGCLocker is enabled when GC locker is started, in another word, at the beginning and end of the GC locker, whether EventGCLocker is enabled may be inconsistent.

So check here if _needs_gc_start_timestamp is not zero, if it is not 0, it needs to reset _needs_gc_start_timestamp regardless of whether the event will be sent.

> src/hotspot/share/gc/shared/gcTraceSend.cpp line 372:
> 
>> 370:     EventGCLocker event(UNTIMED);
>> 371:     if (event.should_commit()) {
>> 372:       event.set_starttime(_needs_gc_start_timestamp);
> 
> Shouldn't you also set the endtime using `event.set_endtime(...)`?

endtime will be set in event.commit()

> src/hotspot/share/gc/shared/gcTraceSend.cpp line 378:
> 
>> 376:     }
>> 377:     _needs_gc_start_timestamp = zero;
>> 378:     _stall_count = 0;
> 
> Any reason to not clear `_jni_lock_count`? It would be needed for the assert suggested above.

There is no special reason, just to save memory access. It's okay for me to reset it.
Fixed.

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

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