Implementation of GC phase pause events #4272

zhengyu123 · 2022-01-28T18:30:59Z

Please review this patch that implements GC pause phase events.

GC pause phase events are emitted during GC phase changes at GC pause. To match hotspot implementation, there can be maximum 5 level (0 - 4) of nesting phases

The sample events:

jdk.GCPhasePause {
  startTime = 12:58:00.232
  duration = 78.384 ms
  gcId = 1
  name = "CollectOnAllocation"
  eventThread = "main" (javaThreadId = 1)
}

jdk.GCPhasePauseLevel1 {
  startTime = 12:58:00.283
  duration = 27.470 ms
  gcId = 1
  name = "Full GC"
  eventThread = "main" (javaThreadId = 1)
}

jdk.GCPhasePauseLevel2 {
  startTime = 12:58:00.311
  duration = 5.775 us
  gcId = 1
  name = "Release Spaces"
  eventThread = "main" (javaThreadId = 1)
}

christianhaeubl · 2022-01-31T10:09:47Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/gc/JfrGCEventSupport.java

+            JfrNativeEventWriter.putEventThread(data);
+            JfrNativeEventWriter.putLong(data, phase.getGCEpoch().rawValue());
+
+            int len = phase.getName().getLength();


The code below can be replaced with JfrNativeEventWriter.putString.

christianhaeubl · 2022-01-31T10:16:13Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/gc/NativeStringConversion.java

+import static jdk.internal.misc.Unsafe.ARRAY_BYTE_BASE_OFFSET;
+import static jdk.internal.misc.Unsafe.ARRAY_BYTE_INDEX_SCALE;
+
+final class NativeStringConversion {


This class should be unnecessary.

christianhaeubl · 2022-01-31T10:16:26Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/gc/NativeString.java

+ * NativeString stores java.lang.String content in native memory
+ */
+@RawStructure
+interface NativeString extends PointerBase {


This interface should be unnecessary.

christianhaeubl · 2022-01-31T10:18:28Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/gc/JfrGCEventSupport.java

+            case 4:
+                return JfrEvents.GCPhasePauseLevel4Event;
+            default:
+                VMError.shouldNotReachHere("At most " + MAX_PHASE_LEVEL + " levels");


throw VMError... avoids the unnecessary return.

christianhaeubl · 2022-01-31T10:19:37Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/gc/JfrGCPausePhase.java

+ * Therefore, we need to keep the states on stack or in native memory.
+ */
+@RawStructure
+public interface JfrGCPausePhase extends PointerBase {


This interface should be unnecessary.

christianhaeubl · 2022-01-31T10:27:22Z

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/GCImpl.java

@@ -207,6 +209,9 @@ private boolean collectImpl(GCCause cause, long requestingNanoTime, boolean forc
        precondition();

        NoAllocationVerifier nav = noAllocationVerifier.open();
+
+        JfrGCPausePhase phase = StackValue.get(JfrGCPausePhase.class);


I think that a large portion of this PR is unnecessary. I don't think that there is any need for extra data structures:

the GC epoch can be queried at any time via getCollectionEpoch()

the start ticks can be stored in a local

the level can be a constant

the phase name can be a String constant

Question: Are constant strings guaranteed in unmovable partition of heap in binary image? I introduced NativeString, not because of this PR, but if I want to generate per-worker events during, e.g. object evacuation phase, I am not sure, but unlikely, there are barriers for accessing the phase names on GC path.

Constant strings are located the image heap and won't be touched by the GC, so they can be used safely. Per-worker events would still use the same constant Strings (only the data in the eventThread field of the JFR events would be different).

christianhaeubl · 2022-02-01T08:46:50Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/JfrThreadLocal.java

@@ -137,6 +142,21 @@ public void afterThreadExit(IsolateThread isolateThread, Thread javaThread) {
        nativeBuffer.set(isolateThread, WordFactory.nullPointer());
    }

+    public void pushPausePhaseLevel() {


In GCImpl, it should be possible to directly use constant instead of a counter for the levels. If that is not feasible for some reason, please add the level as a field to JfrGCEventSupport and use that instead of the thread local.

christianhaeubl · 2022-02-01T08:53:59Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/gc/JfrGCEventSupport.java

+import com.oracle.svm.core.jfr.SubstrateJVM;
+import com.oracle.svm.core.util.VMError;
+
+public abstract class JfrGCEventSupport {


Please move this class to the GC project as only that GC will use this file. There shouldn't be any need to register this class as an ImageSingleton (you can add a new field to GCImpl instead).

You mean moving it to com.oracle.svm.core.genscavenge? Is it the only GC that graal plans to support?

Yes, com.oracle.svm.core.genscavenge.
For now, this is the only GC that will use this particular implementation to trigger JFR events.

christianhaeubl · 2022-02-01T09:01:46Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/gc/JfrGCEventSupport.java

+
+    public abstract void emitGCPausePhaseEvent(UnsignedWord gcEpoch, String name, long startTicks);
+
+    public static class Unsupported extends JfrGCEventSupport {


This extra dummy implementation shouldn't be necessary. You can directly check in each relevant methods if the JFR support is enabled or not (see JfrEnabled.get()). This will get optimized at image build time.

Nice! I thought it is a runtime flag.

JfrEnabled.get() is annotated with @Fold and this annotation ensures that the method is optimized away and replaced with a constant during the image build.

christianhaeubl

The changes look good, please squash your commits if possible. I will then integrate them into master.

zhengyu123 · 2022-02-03T14:07:19Z

The changes look good, please squash your commits if possible. I will then integrate them into master.

Done. The failed tests do not seem to relate to this PR.

Thanks!

zhengyu123 · 2022-02-07T16:39:22Z

All clean now.

christianhaeubl · 2022-02-10T14:57:17Z

This PR was already merged to master - seems that the graalvmbot didn't realize that.

jerboaa · 2022-02-11T17:31:39Z

870990e was the commit I believe.

zhengyu123 marked this pull request as draft January 28, 2022 18:49

graalvmbot added the oca-signed label Jan 28, 2022

jiekang requested review from christianhaeubl and removed request for christianhaeubl January 28, 2022 20:35

christianhaeubl reviewed Jan 31, 2022

View reviewed changes

zhengyu123 marked this pull request as ready for review January 31, 2022 20:29

christianhaeubl reviewed Feb 1, 2022

View reviewed changes

zhengyu123 changed the title ~~Implementation of GC pause phase events~~ Implementation of GC phase pause events Feb 2, 2022

christianhaeubl reviewed Feb 2, 2022

View reviewed changes

zhengyu123 force-pushed the gc_event_impl branch 2 times, most recently from 870990e to 23192ad Compare February 3, 2022 13:31

alina-yur mentioned this pull request Feb 5, 2022

[Discussion / Documentation] Better "roadmap" visibility and discussion for "coming up" #4267

Closed

Implementation of GC phase pause events

a46c6be

zhengyu123 force-pushed the gc_event_impl branch from 23192ad to a46c6be Compare February 7, 2022 15:00

jiekang requested a review from christianhaeubl February 10, 2022 14:52

christianhaeubl closed this Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of GC phase pause events #4272

Implementation of GC phase pause events #4272

zhengyu123 commented Jan 28, 2022 •

edited

Loading

christianhaeubl Jan 31, 2022

christianhaeubl Jan 31, 2022

christianhaeubl Jan 31, 2022

christianhaeubl Jan 31, 2022

christianhaeubl Jan 31, 2022

christianhaeubl Jan 31, 2022

zhengyu123 Jan 31, 2022

christianhaeubl Jan 31, 2022

christianhaeubl Feb 1, 2022

zhengyu123 Feb 2, 2022

christianhaeubl Feb 1, 2022

zhengyu123 Feb 2, 2022

christianhaeubl Feb 2, 2022

christianhaeubl Feb 1, 2022

zhengyu123 Feb 2, 2022

christianhaeubl Feb 2, 2022

christianhaeubl left a comment

zhengyu123 commented Feb 3, 2022

zhengyu123 commented Feb 7, 2022

christianhaeubl commented Feb 10, 2022

jerboaa commented Feb 11, 2022


		public abstract void emitGCPausePhaseEvent(UnsignedWord gcEpoch, String name, long startTicks);

		public static class Unsupported extends JfrGCEventSupport {

Implementation of GC phase pause events #4272

Implementation of GC phase pause events #4272

Conversation

zhengyu123 commented Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianhaeubl left a comment

Choose a reason for hiding this comment

zhengyu123 commented Feb 3, 2022

zhengyu123 commented Feb 7, 2022

christianhaeubl commented Feb 10, 2022

jerboaa commented Feb 11, 2022

zhengyu123 commented Jan 28, 2022 •

edited

Loading