Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GR-51307] Add GC NotificationEmitter Support #9799

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
6 changes: 6 additions & 0 deletions substratevm/mx.substratevm/suite.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,9 @@
"java.management": [
"sun.management",
],
"jdk.management": [
"com.sun.management.internal"
],
"jdk.internal.vm.ci" : [
"jdk.vm.ci.code",
],
Expand Down Expand Up @@ -994,6 +997,9 @@
"jdk.internal.misc",
"sun.security.jca",
],
"java.management": [
"sun.management",
],
},
"checkstyle": "com.oracle.svm.test",
"checkstyleVersion" : "10.7.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/*
* Copyright (c) 2024, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2024, 2024, Red Hat Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package com.oracle.svm.core.genscavenge;

import com.oracle.svm.core.util.BasedOnJDKFile;

import com.oracle.svm.core.SubstrateUtil;
import com.oracle.svm.core.genscavenge.service.GcNotificationRequest;
import com.oracle.svm.core.genscavenge.service.PoolMemoryUsage;
import com.oracle.svm.core.genscavenge.service.Target_com_sun_management_GcInfo;
import com.oracle.svm.core.genscavenge.service.Target_com_sun_management_internal_GcInfoBuilder;

import com.sun.management.GarbageCollectionNotificationInfo;
import com.sun.management.GcInfo;
import com.sun.management.internal.GarbageCollectionNotifInfoCompositeData;
import com.sun.management.internal.GcInfoBuilder;
import sun.management.NotificationEmitterSupport;

import javax.management.MBeanNotificationInfo;
import javax.management.Notification;
import javax.management.NotificationEmitter;
import javax.management.openmbean.CompositeData;
import java.lang.management.MemoryUsage;

public abstract class AbstractGarbageCollectorMXBean extends NotificationEmitterSupport
implements com.sun.management.GarbageCollectorMXBean, NotificationEmitter {

@BasedOnJDKFile("https://github.com/openjdk/jdk/blob/jdk-24+14/src/hotspot/share/gc/serial/serialHeap.cpp#L451") //
private static final String ACTION_MINOR = "end of minor GC";

@BasedOnJDKFile("https://github.com/openjdk/jdk/blob/jdk-24+14/src/hotspot/share/gc/serial/serialHeap.cpp#L718") //
private static final String ACTION_MAJOR = "end of major GC";

private GcInfoBuilder gcInfoBuilder;
private volatile GcInfo gcInfo;
private long seqNumber = 0;

private synchronized GcInfoBuilder getGcInfoBuilder() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
return gcInfoBuilder;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AbstractMemoryPoolMXBean[] beans = GenScavengeMemoryPoolMXBeans.singleton().getMXBeans();

String[] poolNames = getMemoryPoolNames();
MemoryUsage[] before = new MemoryUsage[poolNames.length];
MemoryUsage[] after = new MemoryUsage[poolNames.length];

// Pools must be in the order of getMemoryPoolNames() to match GcInfoBuilder
for (int i = 0; i < poolNames.length; i++) {
for (int j = 0; j < beans.length; j++) {
PoolMemoryUsage pmu = request.getPoolBefore(j);
if (pmu.name != null && pmu.name.equals(poolNames[i])) {
before[i] = beans[j].memoryUsage(pmu.used, pmu.committed);
pmu = request.getPoolAfter(j);
after[i] = beans[j].memoryUsage(pmu.used, pmu.committed);
}
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Target_com_sun_management_GcInfo targetGcInfo = new Target_com_sun_management_GcInfo(getGcInfoBuilder(), request.epoch, request.startTime, request.endTime, before, after, extAttribute);
gcInfo = SubstrateUtil.cast(targetGcInfo, GcInfo.class);

GarbageCollectionNotificationInfo info = new GarbageCollectionNotificationInfo(
getName(),
request.isIncremental ? ACTION_MINOR : ACTION_MAJOR,
request.cause.getName(),
gcInfo);

CompositeData cd = GarbageCollectionNotifInfoCompositeData.toCompositeData(info);

Notification notif = new Notification(GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION,
getObjectName(),
getNextSeqNumber(),
request.timestamp,
getName());
notif.setUserData(cd);

sendNotification(notif);
}

private long getNextSeqNumber() {
return ++seqNumber;
}

@Override
public MBeanNotificationInfo[] getNotificationInfo() {
return new MBeanNotificationInfo[]{
new MBeanNotificationInfo(
new String[]{GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION},
"javax.management.Notification",
"GC Notification")
};
}

@Override
public GcInfo getLastGcInfo() {
return gcInfo;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,28 @@ UnsignedWord getInitialValue() {
return initialValue;
}

public abstract UnsignedWord getUsedBytes();

public UnsignedWord getCommittedBytes() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EdenMemoryPoolMXBean should override this method with something like return HeapImpl.getAccounting().getEdenUsedBytes() + HeapImpl.getAccounting().getBytesInUnusedChunks();.

return getUsedBytes();
}

abstract UnsignedWord computeInitialValue();

abstract void beforeCollection();

abstract void afterCollection();

MemoryUsage memoryUsage(UnsignedWord usedAndCommitted) {
return new MemoryUsage(getInitialValue().rawValue(), usedAndCommitted.rawValue(), usedAndCommitted.rawValue(), getMaximumValue().rawValue());
return memoryUsage(usedAndCommitted, usedAndCommitted);
}

MemoryUsage memoryUsage(UnsignedWord used, UnsignedWord committed) {
return new MemoryUsage(getInitialValue().rawValue(), used.rawValue(), committed.rawValue(), getMaximumValue().rawValue());
return memoryUsage(used.rawValue(), committed.rawValue());
}

public MemoryUsage memoryUsage(long used, long committed) {
return new MemoryUsage(getInitialValue().rawValue(), used, committed, getMaximumValue().rawValue());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,16 @@

import java.lang.management.ManagementFactory;

import javax.management.MBeanNotificationInfo;
import javax.management.NotificationEmitter;
import javax.management.NotificationFilter;
import javax.management.NotificationListener;
import javax.management.ObjectName;

import org.graalvm.nativeimage.Platform;
import org.graalvm.nativeimage.Platforms;

import com.oracle.svm.core.util.TimeUtils;
import com.sun.management.GcInfo;

import sun.management.Util;

public final class CompleteGarbageCollectorMXBean implements com.sun.management.GarbageCollectorMXBean, NotificationEmitter {
public final class CompleteGarbageCollectorMXBean extends AbstractGarbageCollectorMXBean {

@Platforms(Platform.HOSTED_ONLY.class)
public CompleteGarbageCollectorMXBean() {
Expand Down Expand Up @@ -81,26 +76,4 @@ public boolean isValid() {
public ObjectName getObjectName() {
return Util.newObjectName(ManagementFactory.GARBAGE_COLLECTOR_MXBEAN_DOMAIN_TYPE, getName());
}

@Override
public void removeNotificationListener(NotificationListener listener, NotificationFilter filter, Object handback) {
}

@Override
public void addNotificationListener(NotificationListener listener, NotificationFilter filter, Object handback) {
}

@Override
public void removeNotificationListener(NotificationListener listener) {
}

@Override
public MBeanNotificationInfo[] getNotificationInfo() {
return new MBeanNotificationInfo[0];
}

@Override
public GcInfo getLastGcInfo() {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import com.oracle.svm.core.genscavenge.HeapChunk.Header;
import com.oracle.svm.core.genscavenge.UnalignedHeapChunk.UnalignedHeader;
import com.oracle.svm.core.genscavenge.remset.RememberedSet;
import com.oracle.svm.core.genscavenge.service.ServiceSupport;
import com.oracle.svm.core.graal.RuntimeCompilation;
import com.oracle.svm.core.heap.CodeReferenceMapDecoder;
import com.oracle.svm.core.heap.GC;
Expand Down Expand Up @@ -103,6 +104,7 @@
import com.oracle.svm.core.threadlocal.VMThreadLocalSupport;
import com.oracle.svm.core.util.TimeUtils;
import com.oracle.svm.core.util.VMError;
import com.oracle.svm.core.VMInspectionOptions;

import jdk.graal.compiler.api.replacements.Fold;

Expand Down Expand Up @@ -240,6 +242,9 @@ assert getCollectionEpoch().equal(data.getRequestingEpoch()) ||
try {
ThreadLocalAllocation.disableAndFlushForAllThreads();
GenScavengeMemoryPoolMXBeans.singleton().notifyBeforeCollection();
if (VMInspectionOptions.hasGcNotificationSupport()) {
ServiceSupport.singleton().beforeCollection(Isolates.getCurrentUptimeMillis());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to consistently use the values from the collectionTimer instead of Isolates.getCurrentUptimeMillis(). Otherwise, the different mechanism (e.g., -XX:+PrintGC vs. GC notification) may report different pause times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I've update this to use collectionTimer. But I still have to process the time values a bit so that they are ms since the VM started. But at least the GC durations should match now.

}
HeapImpl.getAccounting().notifyBeforeCollection();

verifyHeap(Before);
Expand All @@ -257,6 +262,10 @@ assert getCollectionEpoch().equal(data.getRequestingEpoch()) ||
GenScavengeMemoryPoolMXBeans.singleton().notifyAfterCollection();
ChunkBasedCommittedMemoryProvider.get().afterGarbageCollection();

if (VMInspectionOptions.hasGcNotificationSupport()) {
ServiceSupport.singleton().afterCollection(!isCompleteCollection(), cause, getCollectionEpoch(), Isolates.getCurrentUptimeMillis());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be consistent with other code in that area, please directly access the field completeCollection instead of calling the method isCompleteCollection().

}

printGCAfter(cause);
JfrGCHeapSummaryEvent.emit(JfrGCWhen.AFTER_GC);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@
import jdk.graal.compiler.api.replacements.Fold;

public class GenScavengeMemoryPoolMXBeans {
static final String YOUNG_GEN_SCAVENGER = "young generation scavenger";
static final String COMPLETE_SCAVENGER = "complete scavenger";
public static final String YOUNG_GEN_SCAVENGER = "young generation scavenger";
public static final String COMPLETE_SCAVENGER = "complete scavenger";
static final String EPSILON_SCAVENGER = "epsilon scavenger";

static final String EDEN_SPACE = "eden space";
static final String SURVIVOR_SPACE = "survivor space";
static final String OLD_GEN_SPACE = "old generation space";
public static final String OLD_GEN_SPACE = "old generation space";
static final String EPSILON_HEAP = "epsilon heap";

private final AbstractMemoryPoolMXBean[] mxBeans;
Expand Down Expand Up @@ -111,12 +111,12 @@ UnsignedWord computeInitialValue() {

@Override
public MemoryUsage getUsage() {
return memoryUsage(getCurrentUsage());
return memoryUsage(getUsedBytes());
}

@Override
public MemoryUsage getPeakUsage() {
updatePeakUsage(getCurrentUsage());
updatePeakUsage(getUsedBytes());
return memoryUsage(peakUsage.get());
}

Expand All @@ -125,7 +125,8 @@ public MemoryUsage getCollectionUsage() {
return memoryUsage(WordFactory.zero());
}

private static UnsignedWord getCurrentUsage() {
@Override
public UnsignedWord getUsedBytes() {
return HeapImpl.getAccounting().getEdenUsedBytes();
}
}
Expand Down Expand Up @@ -166,6 +167,11 @@ public MemoryUsage getPeakUsage() {
public MemoryUsage getCollectionUsage() {
return memoryUsage(HeapImpl.getAccounting().getSurvivorUsedBytes());
}

@Override
public UnsignedWord getUsedBytes() {
return HeapImpl.getAccounting().getSurvivorUsedBytes();
}
}

static final class OldGenerationMemoryPoolMXBean extends AbstractMemoryPoolMXBean {
Expand Down Expand Up @@ -204,6 +210,11 @@ public MemoryUsage getPeakUsage() {
public MemoryUsage getCollectionUsage() {
return memoryUsage(HeapImpl.getAccounting().getOldUsedBytes());
}

@Override
public UnsignedWord getUsedBytes() {
return HeapImpl.getAccounting().getOldUsedBytes();
}
}

static final class EpsilonMemoryPoolMXBean extends AbstractMemoryPoolMXBean {
Expand Down Expand Up @@ -243,5 +254,15 @@ public MemoryUsage getPeakUsage() {
public MemoryUsage getCollectionUsage() {
return memoryUsage(WordFactory.zero());
}

@Override
public UnsignedWord getUsedBytes() {
return HeapImpl.getAccounting().getUsedBytes();
}

@Override
public UnsignedWord getCommittedBytes() {
return HeapImpl.getAccounting().getCommittedBytes();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public void notifyAfterCollection() {
invalidData = false;
}

HeapSizes getHeapSizesBeforeGc() {
public HeapSizes getHeapSizesBeforeGc() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this change is needed.

return beforeGc;
}

Expand Down
Loading