Skip to content

[GR-57064] Lazy virtual thread JFR registration and epoch generation caching #9570

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 24, 2025
Merged

[GR-57064] Lazy virtual thread JFR registration and epoch generation caching #9570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43b82b1
lazy vthread registration
roberttoyonaga Aug 21, 2024
e33f0ac
remove epoch boolean
roberttoyonaga Sep 4, 2024
c1e4ab0
style
roberttoyonaga Sep 4, 2024
22add5c
Use putThread to avoid races.
roberttoyonaga Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/JfrThreadRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,6 @@ public void registerRunningThreads() {
Thread thread = PlatformThreads.fromVMThread(isolateThread);
if (thread != null) {
registerThread(thread);
// Re-register vthreads that are already mounted.
Thread vthread = PlatformThreads.getMountedVirtualThread(thread);
if (vthread != null) {
registerThread(vthread);
}
}
}
}
Expand Down
22 changes: 22 additions & 0 deletions substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/SubstrateJVM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

import java.util.List;

import com.oracle.svm.core.SubstrateUtil;
import org.graalvm.nativeimage.ImageSingletons;
import org.graalvm.nativeimage.IsolateThread;
import org.graalvm.nativeimage.Platform;
Expand All @@ -42,13 +43,15 @@
import com.oracle.svm.core.jfr.oldobject.JfrOldObjectRepository;
import com.oracle.svm.core.jfr.sampler.JfrExecutionSampler;
import com.oracle.svm.core.jfr.throttling.JfrEventThrottling;
import com.oracle.svm.core.jfr.traceid.JfrTraceIdEpoch;
import com.oracle.svm.core.log.Log;
import com.oracle.svm.core.sampler.SamplerBufferPool;
import com.oracle.svm.core.sampler.SamplerBuffersAccess;
import com.oracle.svm.core.sampler.SamplerStatistics;
import com.oracle.svm.core.sampler.SubstrateSigprofHandler;
import com.oracle.svm.core.thread.JavaThreads;
import com.oracle.svm.core.thread.JavaVMOperation;
import com.oracle.svm.core.thread.Target_java_lang_VirtualThread;
import com.oracle.svm.core.thread.VMThreads;
import com.oracle.svm.core.util.VMError;

Expand Down Expand Up @@ -303,6 +306,7 @@ public long getStackTraceId(JfrEvent eventType, int skipCount) {
@Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
public static long getThreadId(Thread thread) {
if (HasJfrSupport.get()) {
maybeRegisterVirtualThread(thread);
return JavaThreads.getThreadId(thread);
}
return 0;
Expand All @@ -311,11 +315,29 @@ public static long getThreadId(Thread thread) {
@Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
public static long getCurrentThreadId() {
if (HasJfrSupport.get()) {
maybeRegisterVirtualThread(Thread.currentThread());
return JavaThreads.getCurrentThreadId();
}
return 0;
}

/**
* Register virtual threads if they aren't registered already. Platform threads are registered
* eagerly when started and at chunk rotations.
*/
@Uninterruptible(reason = "Epoch should not change while checking generation.")
private static void maybeRegisterVirtualThread(Thread thread) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this break once we leave uninterruptible code?

  • we start in epoch 0
  • thread A calls getThreadId(...), which registers a vthread for epoch 0 in maybeRegisterVirtualThread
  • thread A leaves the uninterruptible code and gets blocked at a safepoint
  • thread B changes the epoch from 0 to 1
  • after the safepoint, thread A continues execution and uses the thread id that was returned by getThreadId(...) even though the vthread was only registered for epoch 0

One example where I think that this might happen: JavaMonitorQueuedSynchronizer.ConditionNode.notifierJfrTid

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 think the situation you describe is actually ok. Once the epoch changes, new event emissions from "thread A" will fail the epoch generation check, and registration will be re-done. Since getThreadID() is called for every event emission, the important thing is that gathering event info with getThreadID() and the writing of the event data to the JFR buffer are done within the same block of uninterruptible code, which does indeed happen (ex. all emit0 methods).

Calls to getThreadID() outside of the event emission path (ex. with JavaMonitorQueuedSynchronizer.ConditionNode.notifierJfrTid) result in unnecessary registration checks, but shouldn't break anything.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so you essentially assume that getThreadID() will be called again for thread A once the thread ID is actually used in a JFR event. However, I don't think that this assumption is valid. Here is one example:

  • Thread B blocks at ConditionNode.
  • Thread A notifies thread B and sets ConditionNode.notifierJfrTid.
  • The JFR epoch changes.
  • Once thread B wakes up, it uses the value of ConditionNode.notifierJfrTid (i.e., the ID of thread A) when emitting a JavaMonitorWaitEvent. However, thread A is not registered for the current epoch.

Copy link
Collaborator Author

@roberttoyonaga roberttoyonaga Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I see. I mistakenly thought we always used JfrNativeEventWriter.putThread(JfrNativeEventWriterData , Thread ) when dealing with threads. But in that case we use a the plain TID directly.

I think we'll have to switch to always using putThread whenever dealing with the JFR Thread type. What do you think of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The downside is that we'll have to remember to give careful consideration to emitting JFR Thread data.
I'm not sure there's a better place to put the registration check though. And we'll need to check at some point in order to copy Hotspot and do lazy registration.

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 just pushed a commit to update all the places we write thread IDs as JFR event data to use either putThread(Thread) or putThread(long). This should ensure that threads are always registered if they get referenced from event data. The downside is that we need to remember to use putThread.

Another unfortunate thing is that we can only do checks with putThread(Thread) because with only the TID we have no way of knowing whether the thread is virtual and cannot access Target_java_lang_VirtualThread.jfrGeneration.

// Do quick preliminary checks to avoid global locking unless necessary.
if (JavaThreads.isVirtual(thread)) {
Target_java_lang_VirtualThread tjlv = SubstrateUtil.cast(thread, Target_java_lang_VirtualThread.class);
int currentEpochGen = JfrTraceIdEpoch.getInstance().currentEpochGeneration();
if (tjlv.jfrGeneration != currentEpochGen) {
getThreadRepo().registerThread(thread);
tjlv.jfrGeneration = currentEpochGen;
}
}
}

/**
* See {@link JVM#storeMetadataDescriptor}.
*/
Expand Down
8 changes: 4 additions & 4 deletions ...evm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/JavaMonitorEnterEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@
import com.oracle.svm.core.jfr.SubstrateJVM;

public class JavaMonitorEnterEvent {
public static void emit(Object obj, long previousOwnerTid, long startTicks) {
public static void emit(Object obj, Thread previousOwner, long startTicks) {
if (HasJfrSupport.get()) {
emit0(obj, previousOwnerTid, startTicks);
emit0(obj, previousOwner, startTicks);
}
}

@Uninterruptible(reason = "Accesses a JFR buffer.")
public static void emit0(Object obj, long previousOwnerTid, long startTicks) {
public static void emit0(Object obj, Thread previousOwner, long startTicks) {
long duration = JfrTicks.duration(startTicks);
if (JfrEvent.JavaMonitorEnter.shouldEmit(duration)) {
JfrNativeEventWriterData data = StackValue.get(JfrNativeEventWriterData.class);
Expand All @@ -58,7 +58,7 @@ public static void emit0(Object obj, long previousOwnerTid, long startTicks) {
JfrNativeEventWriter.putEventThread(data);
JfrNativeEventWriter.putLong(data, SubstrateJVM.get().getStackTraceId(JfrEvent.JavaMonitorEnter, 0));
JfrNativeEventWriter.putClass(data, obj.getClass());
JfrNativeEventWriter.putLong(data, previousOwnerTid);
JfrNativeEventWriter.putThread(data, previousOwner);
JfrNativeEventWriter.putLong(data, Word.objectToUntrackedPointer(obj).rawValue());
JfrNativeEventWriter.endSmallEvent(data);
}
Expand Down
7 changes: 7 additions & 0 deletions substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/traceid/JfrTraceIdEpoch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public class JfrTraceIdEpoch {
private static final long EPOCH_1_BIT = 0b10;

private boolean epoch;
Copy link
Member

Choose a reason for hiding this comment

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

epoch can probably be removed (i.e., can be computed from the least significant bit of epochGeneration).

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, good idea. I'll remove that.

private int epochGeneration;

@Fold
public static JfrTraceIdEpoch getInstance() {
Expand All @@ -57,6 +58,7 @@ public JfrTraceIdEpoch() {
public void changeEpoch() {
assert VMOperation.isInProgressAtSafepoint();
epoch = !epoch;
epochGeneration++;
}

@Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
Expand All @@ -74,6 +76,11 @@ public boolean currentEpoch() {
return epoch;
}

@Uninterruptible(reason = "Avoid epoch changing while checking generation.", callerMustBe = true)
public int currentEpochGeneration() {
return epochGeneration;
}

@Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
public boolean previousEpoch() {
return !epoch;
Expand Down
9 changes: 4 additions & 5 deletions substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/JavaMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@

import com.oracle.svm.core.Uninterruptible;
import com.oracle.svm.core.jfr.JfrTicks;
import com.oracle.svm.core.jfr.SubstrateJVM;
import com.oracle.svm.core.jfr.events.JavaMonitorEnterEvent;
import com.oracle.svm.core.thread.JavaThreads;
import com.oracle.svm.core.util.BasedOnJDKClass;
Expand All @@ -61,20 +60,20 @@
@BasedOnJDKClass(ReentrantLock.class)
@BasedOnJDKClass(value = ReentrantLock.class, innerClass = "Sync")
public class JavaMonitor extends JavaMonitorQueuedSynchronizer {
protected long latestJfrTid;
protected Thread latest;

public JavaMonitor() {
latestJfrTid = 0;
latest = null;
}

public void monitorEnter(Object obj) {
if (!tryLock()) {
long startTicks = JfrTicks.elapsedTicks();
acquire(1);
JavaMonitorEnterEvent.emit(obj, latestJfrTid, startTicks);
JavaMonitorEnterEvent.emit(obj, latest, startTicks);
}

latestJfrTid = SubstrateJVM.getCurrentThreadId();
latest = Thread.currentThread();
}

public void monitorExit() {
Expand Down
2 changes: 1 addition & 1 deletion .../src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ public void monitorEnter(Object obj, MonitorInflationCause cause) {
monitor = (JavaMonitor) UNSAFE.compareAndExchangeReference(obj, monitorOffset, null, newMonitor);
if (monitor == null) { // successful
JavaMonitorInflateEvent.emit(obj, startTicks, MonitorInflationCause.MONITOR_ENTER);
newMonitor.latestJfrTid = current;
newMonitor.latest = Thread.currentThread();
return;
}
}
Expand Down
9 changes: 4 additions & 5 deletions ...rc/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@
import com.oracle.svm.core.annotate.TargetElement;
import com.oracle.svm.core.jdk.JDK21OrEarlier;
import com.oracle.svm.core.jdk.JDKLatest;
import com.oracle.svm.core.jfr.HasJfrSupport;
import com.oracle.svm.core.jfr.SubstrateJVM;
import com.oracle.svm.core.monitor.MonitorInflationCause;
import com.oracle.svm.core.monitor.MonitorSupport;
import com.oracle.svm.core.util.VMError;
Expand Down Expand Up @@ -113,6 +111,10 @@ public final class Target_java_lang_VirtualThread {
private Executor nondefaultScheduler;
// Checkstyle: resume

@Inject //
@RecomputeFieldValue(kind = RecomputeFieldValue.Kind.Reset) //
public volatile int jfrGeneration = -1;

@Alias
private static native ForkJoinPool createDefaultScheduler();

Expand Down Expand Up @@ -330,9 +332,6 @@ void mount() {
}

carrier.setCurrentThread(asThread(this));
if (HasJfrSupport.get()) {
SubstrateJVM.getThreadRepo().registerThread(asThread(this));
}
}

@Substitute // not needed on newer JDKs that use safe disableSuspendAndPreempt()
Expand Down