RFR: 8259808: Add JFR event to detect GC locker stall

[Stefan Karlsson]

On Fri, 15 Jan 2021 02:42:20 GMT, Denghui Dong <ddong at openjdk.org> wrote:

> GC locker will stall the operation of GC, resulting in some Java threads can not continue to run until GC locker is released, thus affecting the response time of the application. Add a JFR event to report this information is helpful to detect this issue.
> 
> For the test purpose, I add two Whitebox methods to lock/unlock critical.

Not a review, but a few comments about what probably needs to be cleaned up before a proper review starts.

src/hotspot/share/gc/shared/gcLocker.cpp line 186:

> 184:       _stall_count = 0;
> 185:     }
> 186: #endif

This adds a fair amount of noise and hides the actual GCLocker logic, IMHO. Could you somehow encapsulate this code and the other INCLUDE_JFR above into a class and make single-line calls perform these functions?

src/hotspot/share/jfr/metadata/metadata.xml line 1080:

> 1078:     <Field type="uint" name="stallCount" label="Stall Count" />
> 1079:   </Event>
> 1080: 

You add this between two Shenandoah events. Could you put it somewhere where it's not splitting up a group of events?

src/hotspot/share/prims/whitebox.cpp line 44:

> 42: #include "gc/shared/genArguments.hpp"
> 43: #include "gc/shared/genCollectedHeap.hpp"
> 44: #include "gc/shared/gcLocker.inline.hpp"

Sort includes.

src/hotspot/share/gc/shared/gcLocker.cpp line 112:

> 110: #if INCLUDE_JFR
> 111:     if (EventGCLocker::is_enabled()) {
> 112:       _needs_gc_start_timestamp = JfrTicks::now();

Do you really need to use JfrTicks instead of Ticks here? If not, could you remove all references and includes of JfrTicks? We usually use pass in Ticks when we send JFR events.

src/hotspot/share/utilities/ticks.hpp line 241:

> 239:   friend class GCTimerTest;
> 240:   friend class CompilerEvent;
> 241:   friend class GCLocker;

I don't think this should be needed.

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

Changes requested by stefank (Reviewer).

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