Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions make/data/hotspot-symbols/symbols-unix
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ JVM_VirtualThreadEnd
JVM_VirtualThreadMount
JVM_VirtualThreadUnmount
JVM_VirtualThreadHideFrames
JVM_VirtualThreadDisableSuspend

# Scoped values
JVM_EnsureMaterializedForStackWalk_func
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/classfile/vmIntrinsics.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,7 @@ class methodHandle;
do_intrinsic(_notifyJvmtiVThreadMount, java_lang_VirtualThread, notifyJvmtiMount_name, bool_void_signature, F_RN) \
do_intrinsic(_notifyJvmtiVThreadUnmount, java_lang_VirtualThread, notifyJvmtiUnmount_name, bool_void_signature, F_RN) \
do_intrinsic(_notifyJvmtiVThreadHideFrames, java_lang_VirtualThread, notifyJvmtiHideFrames_name, bool_void_signature, F_RN) \
do_intrinsic(_notifyJvmtiVThreadDisableSuspend, java_lang_VirtualThread, notifyJvmtiDisableSuspend_name, bool_void_signature, F_RN) \
\
/* support for UnsafeConstants */ \
do_class(jdk_internal_misc_UnsafeConstants, "jdk/internal/misc/UnsafeConstants") \
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/classfile/vmSymbols.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,7 @@ class SerializeClosure;
template(notifyJvmtiMount_name, "notifyJvmtiMount") \
template(notifyJvmtiUnmount_name, "notifyJvmtiUnmount") \
template(notifyJvmtiHideFrames_name, "notifyJvmtiHideFrames") \
template(notifyJvmtiDisableSuspend_name, "notifyJvmtiDisableSuspend") \
template(doYield_name, "doYield") \
template(enter_name, "enter") \
template(enterSpecial_name, "enterSpecial") \
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/include/jvm.h
Original file line number Diff line number Diff line change
Expand Up @@ -1154,6 +1154,9 @@ JVM_VirtualThreadUnmount(JNIEnv* env, jobject vthread, jboolean hide);
JNIEXPORT void JNICALL
JVM_VirtualThreadHideFrames(JNIEnv* env, jobject vthread, jboolean hide);

JNIEXPORT void JNICALL
JVM_VirtualThreadDisableSuspend(JNIEnv* env, jobject vthread, jboolean enter);

/*
* Core reflection support.
*/
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/jvmci/vmStructs_jvmci.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@
nonstatic_field(JavaThread, _lock_stack, LockStack) \
JVMTI_ONLY(nonstatic_field(JavaThread, _is_in_VTMS_transition, bool)) \
JVMTI_ONLY(nonstatic_field(JavaThread, _is_in_tmp_VTMS_transition, bool)) \
JVMTI_ONLY(nonstatic_field(JavaThread, _is_disable_suspend, bool)) \
\
nonstatic_field(LockStack, _top, uint32_t) \
\
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/opto/c2compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,7 @@ bool C2Compiler::is_intrinsic_supported(vmIntrinsics::ID id) {
case vmIntrinsics::_notifyJvmtiVThreadMount:
case vmIntrinsics::_notifyJvmtiVThreadUnmount:
case vmIntrinsics::_notifyJvmtiVThreadHideFrames:
case vmIntrinsics::_notifyJvmtiVThreadDisableSuspend:
#endif
break;

Expand Down
26 changes: 25 additions & 1 deletion src/hotspot/share/opto/library_call.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,8 @@ bool LibraryCallKit::try_to_inline(int predicate) {
"notifyJvmtiMount", false, false);
case vmIntrinsics::_notifyJvmtiVThreadUnmount: return inline_native_notify_jvmti_funcs(CAST_FROM_FN_PTR(address, OptoRuntime::notify_jvmti_vthread_unmount()),
"notifyJvmtiUnmount", false, false);
case vmIntrinsics::_notifyJvmtiVThreadHideFrames: return inline_native_notify_jvmti_hide();
case vmIntrinsics::_notifyJvmtiVThreadHideFrames: return inline_native_notify_jvmti_hide();
case vmIntrinsics::_notifyJvmtiVThreadDisableSuspend: return inline_native_notify_jvmti_sync();
#endif

#ifdef JFR_HAVE_INTRINSICS
Expand Down Expand Up @@ -2950,6 +2951,29 @@ bool LibraryCallKit::inline_native_notify_jvmti_hide() {
return true;
}

// Always update the is_disable_suspend bit.
bool LibraryCallKit::inline_native_notify_jvmti_sync() {
if (!DoJVMTIVirtualThreadTransitions) {
return true;
}
IdealKit ideal(this);

{
// unconditionally update the is_disable_suspend bit in current JavaThread
Node* thread = ideal.thread();
Node* arg = _gvn.transform(argument(1)); // argument for notification
Node* addr = basic_plus_adr(thread, in_bytes(JavaThread::is_disable_suspend_offset()));
const TypePtr *addr_type = _gvn.type(addr)->isa_ptr();

sync_kit(ideal);
access_store_at(nullptr, addr, addr_type, arg, _gvn.type(arg), T_BOOLEAN, IN_NATIVE | MO_UNORDERED);
ideal.sync_kit(this);
}
final_sync(ideal);

return true;
}

#endif // INCLUDE_JVMTI

#ifdef JFR_HAVE_INTRINSICS
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/opto/library_call.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ class LibraryCallKit : public GraphKit {
#if INCLUDE_JVMTI
bool inline_native_notify_jvmti_funcs(address funcAddr, const char* funcName, bool is_start, bool is_end);
bool inline_native_notify_jvmti_hide();
bool inline_native_notify_jvmti_sync();
#endif

#ifdef JFR_HAVE_INTRINSICS
Expand Down
16 changes: 16 additions & 0 deletions src/hotspot/share/prims/jvm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4008,6 +4008,22 @@ JVM_ENTRY(void, JVM_VirtualThreadHideFrames(JNIEnv* env, jobject vthread, jboole
#endif
JVM_END

// Notification from VirtualThread about disabling JVMTI Suspend in a sync critical section.
// Needed to avoid deadlocks with JVMTI suspend mechanism.
JVM_ENTRY(void, JVM_VirtualThreadDisableSuspend(JNIEnv* env, jobject vthread, jboolean enter))
#if INCLUDE_JVMTI
if (!DoJVMTIVirtualThreadTransitions) {
assert(!JvmtiExport::can_support_virtual_threads(), "sanity check");
return;
}
assert(thread->is_disable_suspend() != (bool)enter,
"nested or unbalanced monitor enter/exit is not allowed");
thread->toggle_is_disable_suspend();
#else
fatal("Should only be called with JVMTI enabled");
#endif
Comment on lines +4022 to +4024
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't do this! The Java code knows nothing about JVM TI being enabled/disabled and will call this function unconditionally.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't do this! The Java code knows nothing about JVM TI being enabled/disabled and will call this function unconditionally.

Indeed. I wonder if anyone is testing minimal builds to catch issues like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, David!
Filed a cleanup bug: https://bugs.openjdk.org/browse/JDK-8322538

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these new compiler intrinsics required or an optional performance optimization? This PR creates issues for us when updating the JDK build for Graal. CC @davleopo

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these new compiler intrinsics required or an optional performance optimization?

Performance. If the intrinsic isn't there then some methods executed on virtual threads, or on a virtual thread as the target for some op, will have to call into the VM. The main concern was Thread.interrupted() as it gets called very frequently in locking and concurrency code.

JVM_END

/*
* Return the current class's class file version. The low order 16 bits of the
* returned jint contain the class's major version. The high order 16 bits
Expand Down
4 changes: 4 additions & 0 deletions src/hotspot/share/runtime/handshake.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,10 @@ HandshakeOperation* HandshakeState::get_op_for_self(bool allow_suspend, bool che
assert(_handshakee == Thread::current(), "Must be called by self");
assert(_lock.owned_by_self(), "Lock must be held");
assert(allow_suspend || !check_async_exception, "invalid case");
if (allow_suspend && _handshakee->is_disable_suspend()) {
// filter out suspend operations while JavaThread is in disable_suspend mode
allow_suspend = false;
}
if (!allow_suspend) {
return _queue.peek(no_suspend_no_async_exception_filter);
} else if (check_async_exception && !_async_exceptions_blocked) {
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/runtime/javaThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ JavaThread::JavaThread() :
_carrier_thread_suspended(false),
_is_in_VTMS_transition(false),
_is_in_tmp_VTMS_transition(false),
_is_disable_suspend(false),
#ifdef ASSERT
_is_VTMS_transition_disabler(false),
#endif
Expand Down
5 changes: 5 additions & 0 deletions src/hotspot/share/runtime/javaThread.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ class JavaThread: public Thread {
volatile bool _carrier_thread_suspended; // Carrier thread is externally suspended
bool _is_in_VTMS_transition; // thread is in virtual thread mount state transition
bool _is_in_tmp_VTMS_transition; // thread is in temporary virtual thread mount state transition
bool _is_disable_suspend; // JVMTI suspend is temporarily disabled; used on current thread only
#ifdef ASSERT
bool _is_VTMS_transition_disabler; // thread currently disabled VTMS transitions
#endif
Expand Down Expand Up @@ -647,6 +648,9 @@ class JavaThread: public Thread {
void set_is_in_VTMS_transition(bool val);
void toggle_is_in_tmp_VTMS_transition() { _is_in_tmp_VTMS_transition = !_is_in_tmp_VTMS_transition; };

bool is_disable_suspend() const { return _is_disable_suspend; }
void toggle_is_disable_suspend() { _is_disable_suspend = !_is_disable_suspend; };
Copy link
Contributor

@AlanBateman AlanBateman Dec 20, 2023

Choose a reason for hiding this comment

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

Looking at this again then I don't think it can be a bit that is toggled on and off will work. Consider the case where several threads attempt to poll the state of a virtual Thread with Thread::getState at the same time. This can't work without an atomic counter and further coordination. So I think further work is required on this issue.

Update: ignore this I mis-read that it updates the current thread's suspend value, not the thread's suspend value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: ignore this I mis-read that it updates the current thread's suspend value, not the thread's suspend value.

Thanks, Alan. I've also got confused with this and even filed a follow up bug. :)
Yes, the initial design was the _is_disable_suspend is set/modified/accessed on the current thread only.


#ifdef ASSERT
bool is_VTMS_transition_disabler() const { return _is_VTMS_transition_disabler; }
void set_is_VTMS_transition_disabler(bool val);
Expand Down Expand Up @@ -811,6 +815,7 @@ class JavaThread: public Thread {
#if INCLUDE_JVMTI
static ByteSize is_in_VTMS_transition_offset() { return byte_offset_of(JavaThread, _is_in_VTMS_transition); }
static ByteSize is_in_tmp_VTMS_transition_offset() { return byte_offset_of(JavaThread, _is_in_tmp_VTMS_transition); }
static ByteSize is_disable_suspend_offset() { return byte_offset_of(JavaThread, _is_disable_suspend); }
#endif

// Returns the jni environment for this thread
Expand Down
82 changes: 55 additions & 27 deletions src/java.base/share/classes/java/lang/VirtualThread.java
Original file line number Diff line number Diff line change
Expand Up @@ -743,11 +743,16 @@ void unpark() {
}
} else if ((s == PINNED) || (s == TIMED_PINNED)) {
// unpark carrier thread when pinned
synchronized (carrierThreadAccessLock()) {
Thread carrier = carrierThread;
if (carrier != null && ((s = state()) == PINNED || s == TIMED_PINNED)) {
U.unpark(carrier);
notifyJvmtiDisableSuspend(true);
try {
synchronized (carrierThreadAccessLock()) {
Thread carrier = carrierThread;
if (carrier != null && ((s = state()) == PINNED || s == TIMED_PINNED)) {
U.unpark(carrier);
}
}
} finally {
notifyJvmtiDisableSuspend(false);
}
}
}
Expand Down Expand Up @@ -844,16 +849,21 @@ boolean joinNanos(long nanos) throws InterruptedException {
public void interrupt() {
if (Thread.currentThread() != this) {
checkAccess();
synchronized (interruptLock) {
interrupted = true;
Interruptible b = nioBlocker;
if (b != null) {
b.interrupt(this);
}
notifyJvmtiDisableSuspend(true);
try {
synchronized (interruptLock) {
interrupted = true;
Interruptible b = nioBlocker;
if (b != null) {
b.interrupt(this);
}

// interrupt carrier thread if mounted
Thread carrier = carrierThread;
if (carrier != null) carrier.setInterrupt();
// interrupt carrier thread if mounted
Thread carrier = carrierThread;
if (carrier != null) carrier.setInterrupt();
}
} finally {
notifyJvmtiDisableSuspend(false);
}
} else {
interrupted = true;
Expand All @@ -872,9 +882,14 @@ boolean getAndClearInterrupt() {
assert Thread.currentThread() == this;
boolean oldValue = interrupted;
if (oldValue) {
synchronized (interruptLock) {
interrupted = false;
carrierThread.clearInterrupt();
notifyJvmtiDisableSuspend(true);
try {
synchronized (interruptLock) {
interrupted = false;
carrierThread.clearInterrupt();
}
} finally {
notifyJvmtiDisableSuspend(false);
}
}
return oldValue;
Expand All @@ -899,11 +914,16 @@ Thread.State threadState() {
return Thread.State.RUNNABLE;
case RUNNING:
// if mounted then return state of carrier thread
synchronized (carrierThreadAccessLock()) {
Thread carrierThread = this.carrierThread;
if (carrierThread != null) {
return carrierThread.threadState();
notifyJvmtiDisableSuspend(true);
try {
synchronized (carrierThreadAccessLock()) {
Thread carrierThread = this.carrierThread;
if (carrierThread != null) {
return carrierThread.threadState();
}
}
} finally {
notifyJvmtiDisableSuspend(false);
}
// runnable, mounted
return Thread.State.RUNNABLE;
Expand Down Expand Up @@ -1019,14 +1039,19 @@ public String toString() {
Thread carrier = carrierThread;
if (carrier != null) {
// include the carrier thread state and name when mounted
synchronized (carrierThreadAccessLock()) {
carrier = carrierThread;
if (carrier != null) {
String stateAsString = carrier.threadState().toString();
sb.append(stateAsString.toLowerCase(Locale.ROOT));
sb.append('@');
sb.append(carrier.getName());
notifyJvmtiDisableSuspend(true);
try {
synchronized (carrierThreadAccessLock()) {
carrier = carrierThread;
if (carrier != null) {
String stateAsString = carrier.threadState().toString();
sb.append(stateAsString.toLowerCase(Locale.ROOT));
sb.append('@');
sb.append(carrier.getName());
}
}
} finally {
notifyJvmtiDisableSuspend(false);
}
}
// include virtual thread state when not mounted
Expand Down Expand Up @@ -1125,6 +1150,9 @@ private void setCarrierThread(Thread carrier) {
@JvmtiMountTransition
private native void notifyJvmtiHideFrames(boolean hide);

@IntrinsicCandidate
private native void notifyJvmtiDisableSuspend(boolean enter);

private static native void registerNatives();
static {
registerNatives();
Expand Down
11 changes: 6 additions & 5 deletions src/java.base/share/native/libjava/VirtualThread.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,12 @@
#define VIRTUAL_THREAD "Ljava/lang/VirtualThread;"

static JNINativeMethod methods[] = {
{ "notifyJvmtiStart", "()V", (void *)&JVM_VirtualThreadStart },
{ "notifyJvmtiEnd", "()V", (void *)&JVM_VirtualThreadEnd },
{ "notifyJvmtiMount", "(Z)V", (void *)&JVM_VirtualThreadMount },
{ "notifyJvmtiUnmount", "(Z)V", (void *)&JVM_VirtualThreadUnmount },
{ "notifyJvmtiHideFrames", "(Z)V", (void *)&JVM_VirtualThreadHideFrames },
{ "notifyJvmtiStart", "()V", (void *)&JVM_VirtualThreadStart },
{ "notifyJvmtiEnd", "()V", (void *)&JVM_VirtualThreadEnd },
{ "notifyJvmtiMount", "(Z)V", (void *)&JVM_VirtualThreadMount },
{ "notifyJvmtiUnmount", "(Z)V", (void *)&JVM_VirtualThreadUnmount },
{ "notifyJvmtiHideFrames", "(Z)V", (void *)&JVM_VirtualThreadHideFrames },
{ "notifyJvmtiDisableSuspend", "(Z)V", (void *)&JVM_VirtualThreadDisableSuspend },
};

JNIEXPORT void JNICALL
Expand Down
Loading