[GR-51307] Add GC NotificationEmitter Support #9799

roberttoyonaga · 2024-10-01T21:04:43Z

Summary

This PR adds support for the emission of GC notifications from GC MXBeans. Previously, SubstratVM's GC MXBeans (CompleteGarbageCollectorMXBean and IncrementalGarbageCollectorMXBean) implemented NotificationEmitter, but actually did not support notifications.

Details

A new feature ServiceFeature has been added. Use --enable-monitoring=gcnotif to include the feature at build time and add support for GC notifications.

The ImageSingleton ServiceSupport starts up a dedicated service thread which asynchronously handles creating and sending notifications. Notification "requests" are created during GC safepoints. They temporarily hold the information later needed to create and send GC notifications. During the GC safepoint, the thread executing the VM operation enqueues a request in GcNotifier and signals the service thread that a request is ready. That process should be relatively quick and allocation free. Once the safepoint is over, the service thread uses the queued requests to create notifications that are sent by the appropriate GC MXBean.

The new classes GcNotifier and ServiceThread are meant to serve almost the same purposes as the GCNotifier and ServiceThread classes in Hotspot respectively.

The various GC MXBeans now extend AbstractGarbageCollectorMXBean which contains the code for creating and sending notifications from request data. This class serves a similar purpose to com.sun.management.internal.GarbageCollectorExtImpl. Additionally, this class extends sun.management.NotificationEmitterSupport from which the listener registration/deregistration logic is reused.

Other notes

I'm not sure about the naming of ServiceSupport and ServiceFeature. Maybe NotificationSupport makes more sense? I decided on the "service" prefix because, in Hotspot, the GC notification code is in the /hotspot/share/services directory. Also, maybe we will want to add other services in the future.

Related Issues: #8237

christianhaeubl

Thanks for the PR, I added a larger number of comments.

christianhaeubl · 2024-10-09T13:09:23Z

...svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/AbstractGarbageCollectorMXBean.java

+    private volatile GcInfo gcInfo;
+    private long seqNumber = 0;
+
+    private synchronized GcInfoBuilder getGcInfoBuilder() {


I don't think that this need to be synchronized (only called by the service thread).

Please move private helper methods to a place that is below their caller (they are not super important, so they are usually not the first code that someone who opens that class wants to see).

christianhaeubl · 2024-10-09T13:15:03Z

...svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/AbstractGarbageCollectorMXBean.java

+    private synchronized GcInfoBuilder getGcInfoBuilder() {
+        if (gcInfoBuilder == null) {
+            Target_com_sun_management_internal_GcInfoBuilder gib = new Target_com_sun_management_internal_GcInfoBuilder(this, getMemoryPoolNames());
+            gcInfoBuilder = SubstrateUtil.cast(gib, GcInfoBuilder.class);


You can avoid this cast if you use Target_com_sun_management_internal_GcInfoBuilder instead of GcInfoBuilder everywhere (including in the constructor of Target_com_sun_management_GcInfo).

christianhaeubl · 2024-10-09T13:33:19Z

...svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/AbstractGarbageCollectorMXBean.java

+
+    @Override
+    public GcInfo getLastGcInfo() {
+        return gcInfo;


This returns the last gcInfo that was processed by the service thread (i.e., new information will be available with a delay). Is the behavior the same on HotSpot or do they always report up-to-date data?

Oh that's a good point. And it's worse if the queue has more items in it. It looks like Hotspot always returns info from the most recent GC. I'll change this method to do the same.

I've fixed the approach here by always caching the GC info during the collection. This is basically what Hotspot is doing too.

christianhaeubl · 2024-10-09T13:37:47Z

...svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/AbstractGarbageCollectorMXBean.java

+        }
+
+        // Number of GC threads.
+        Object[] extAttribute = new Object[]{Integer.valueOf(1)};


In this particular case, I would prefer something like the following, as it makes things clearer and more readable:

Object[] extAttribute = new Object[1]; extAttribute[0] = 1; // number of GC threads

christianhaeubl · 2024-10-09T15:20:10Z

....oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/service/ServiceSupport.java

+import org.graalvm.word.UnsignedWord;
+
+public class ServiceSupport {
+    private GcNotifier gcNotifier;


This field and serviceThread can be final.

christianhaeubl · 2024-10-10T10:22:31Z

.../com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/service/GcNotifier.java

+         * We don't need to drain all requests since signals queue at the semaphore level. The rest
+         * of the requests will be handled in subsequent calls to this method by the sevice thread.
+         */
+        if (!updateCurrentRequest()) {


Please adapt this logic once you switch to VMMutex and VMCondition.

This is now handled by checking NotificationThread.hasRequests() before blocking in the notification thread loop.

christianhaeubl · 2024-10-10T10:23:50Z

...m.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/service/ServiceThread.java

+ * This class is the dedicated thread that handles services. For now, the only service is GC
+ * notifications.
+ */
+public class ServiceThread extends Thread {


Does HotSpot use the service thread or a separate notification thread for the GC notifications?

Hotspot uses NotificationThread, which is different than ServiceThread. Should I rename this class "NotificationThread" instead? Unless we plan to have a single dedicated thread handle service and notifications.

Renamed everything to notification*

christianhaeubl · 2024-10-10T10:24:28Z

...m.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/service/ServiceThread.java

+
+    @Platforms(Platform.HOSTED_ONLY.class)
+    @SuppressWarnings("this-escape")
+    public ServiceThread(GcNotifier gcNotifier) {


Please set a name for this thread, and potentially also a thread group (please check in the HotSpot code).

christianhaeubl · 2024-10-10T10:38:25Z

...svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/AbstractGarbageCollectorMXBean.java

+     * Use the data taken from the request queue to populate MemoryUsage. The service thread calls
+     * this method.
+     */
+    public void createNotification(GcNotificationRequest request) {


Parts of this code are probably based on OpenJDK code. If so, please annotate with @BasedOnJDKFile.

christianhaeubl · 2024-10-10T10:45:12Z

....svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/service/GcNotificationRequest.java

+ * questions.
+ */
+
+package com.oracle.svm.core.genscavenge.service;


Please move everything in this package to a sub package in the GC independent project com.oracle.svm.core.

I tried doing that originally, but it seems I cannot import from com.oracle.svm.core.genscavenge from inside com.oracle.svm.core. Is there a way to work around this?

The build error:
error: package com.oracle.svm.core.genscavenge does not exist

You will need to split the implementation into a GC independent and a GC-specific part. This will also make it easier to implement this for all Native Image GCs in the future:

GC independent parts in com.oracle.svm.core: notification thread, notification logic, and all the GC independent logic that accesses the memory pools. You will probably need introduce an interface and implementation that interface on the GC-specific side.

GC specific parts in com.oracle.svm.core.genscavenge: essentially everything that would be different for a completely different GC implementation (e.g., the logic in GenScavengeMemoryPoolMXBeans, how many memory pools a GC uses, or how the memory pools are named)

I put the files into com.oracle.svm.core.notification and com.oracle.svm.core.gc (the naming could maybe be improved).

com.oracle.svm.core.gc contains the interface MemoryPoolMXBeansProvider for accessing GC-specific memory pool info.

I also put AbstractGarbageCollectorMXBean inside com.oracle.svm.core.gc. Different GCs can override methods like gcThreadCount() and isIncremental() so that the notification infrastructure knows how to handle notifications for different collectors.

roberttoyonaga · 2024-10-18T19:47:31Z

Thank you for the review @christianhaeubl. I think I've addressed all your comments. Please have another look when you have time. Thanks!

christianhaeubl · 2024-10-25T15:23:20Z

Thanks. I started integrating this PR. I will let you know if I run into any major issues.

roberttoyonaga · 2024-10-25T15:32:01Z

Thanks @christianhaeubl !

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 1, 2024

roberttoyonaga added native-image redhat-interest labels Oct 1, 2024

roberttoyonaga force-pushed the GR-51307 branch 2 times, most recently from 3377f06 to 5e2e82e Compare October 2, 2024 13:36

GC notifications

1923bf4

roberttoyonaga force-pushed the GR-51307 branch from 5e2e82e to 1923bf4 Compare October 2, 2024 13:50

roberttoyonaga mentioned this pull request Oct 2, 2024

[GR-51307] Unable to collect GC data with NotificationEmitter in native build #8237

Open

roberttoyonaga marked this pull request as ready for review October 2, 2024 14:26

don't emit notifications if there are no requests left. Teardown bug.

109d8b9

roberttoyonaga requested a review from christianhaeubl October 7, 2024 14:09

christianhaeubl changed the title ~~Add GC NotificationEmitter Support~~ [GR-51307] Add GC NotificationEmitter Support Oct 10, 2024

christianhaeubl reviewed Oct 10, 2024

View reviewed changes

roberttoyonaga added 11 commits October 11, 2024 12:22

fix minor feedback.

b54ae56

split into GC independent/dependent code

d56a758

refactor queue classes

299dcfc

use mutex+condition, fix getLastGcInfo, use RingBuffer

cb33422

BasedOnJdkFile. Fix signalling

5302406

Merge branch 'master' of github.com:roberttoyonaga/graal into GR-51307

24494be

small cleanup

eb28fe6

gate fixes

4c09b4f

style

df2d4b2

Fix getLastGcInfo when no GC has happened. update tests.

71f75a8

clean suite.py

09890a7

roberttoyonaga mentioned this pull request Oct 21, 2024

[Native Image] jvm_gc_* and jvm_buffer_* metrics missing #9912

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GR-51307] Add GC NotificationEmitter Support #9799

[GR-51307] Add GC NotificationEmitter Support #9799

roberttoyonaga commented Oct 1, 2024 •

edited

Loading

christianhaeubl left a comment

christianhaeubl Oct 9, 2024

christianhaeubl Oct 9, 2024

christianhaeubl Oct 9, 2024

roberttoyonaga Oct 10, 2024

roberttoyonaga Oct 18, 2024

christianhaeubl Oct 9, 2024

christianhaeubl Oct 9, 2024

christianhaeubl Oct 10, 2024

roberttoyonaga Oct 18, 2024

christianhaeubl Oct 10, 2024

roberttoyonaga Oct 10, 2024 •

edited

Loading

roberttoyonaga Oct 18, 2024

christianhaeubl Oct 10, 2024

christianhaeubl Oct 10, 2024

christianhaeubl Oct 10, 2024 •

edited

Loading

roberttoyonaga Oct 10, 2024

christianhaeubl Oct 11, 2024

roberttoyonaga Oct 18, 2024

roberttoyonaga commented Oct 18, 2024

christianhaeubl commented Oct 25, 2024

roberttoyonaga commented Oct 25, 2024

[GR-51307] Add GC NotificationEmitter Support #9799

Are you sure you want to change the base?

[GR-51307] Add GC NotificationEmitter Support #9799

Conversation

roberttoyonaga commented Oct 1, 2024 • edited Loading

Summary

Details

Other notes

christianhaeubl left a 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

roberttoyonaga Oct 10, 2024 • 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

christianhaeubl Oct 10, 2024 • 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

roberttoyonaga commented Oct 18, 2024

christianhaeubl commented Oct 25, 2024

roberttoyonaga commented Oct 25, 2024

roberttoyonaga commented Oct 1, 2024 •

edited

Loading

roberttoyonaga Oct 10, 2024 •

edited

Loading

christianhaeubl Oct 10, 2024 •

edited

Loading