From 0863dba01a1126e7ee34adee6d127010377f26df Mon Sep 17 00:00:00 2001 From: zhangxiaomin03 Date: Thu, 4 Jan 2024 10:10:16 +0800 Subject: [PATCH] [Wisp] Remove CoroutineSupport Lock in Java and unnecessary function coroutineMove. Summary: In Java17, we protect double linked list of coroutine in C++. So there is no need to use Locker in Java to protect it. And also, function moveCoroutine is unnecessary, so we should remove it. Refine nmethods_do mark active. Test Plan: all wisp tests Reviewed-by: yulei Issue: https://github.com/dragonwell-project/dragonwell17/issues/189 --- src/hotspot/share/prims/unsafe.cpp | 25 +-- src/hotspot/share/runtime/sweeper.cpp | 7 +- src/hotspot/share/runtime/sweeper.hpp | 1 + src/hotspot/share/runtime/thread.cpp | 6 + .../com/alibaba/wisp/engine/WispTask.java | 2 +- .../share/classes/java/dyn/CoroutineBase.java | 10 +- .../classes/java/dyn/CoroutineSupport.java | 159 ++---------------- 7 files changed, 36 insertions(+), 174 deletions(-) diff --git a/src/hotspot/share/prims/unsafe.cpp b/src/hotspot/share/prims/unsafe.cpp index a90ef5328a6..ddebf5cf0a2 100644 --- a/src/hotspot/share/prims/unsafe.cpp +++ b/src/hotspot/share/prims/unsafe.cpp @@ -968,14 +968,6 @@ JVM_ENTRY (jobject, CoroutineSupport_getNextCoroutine(JNIEnv* env, jclass klass, return JNIHandles::make_local(THREAD, coro->next()->coroutine()); JVM_END -JVM_ENTRY (void, CoroutineSupport_moveCoroutine(JNIEnv* env, jclass klass, jlong coroPtr, jlong targetPtr)) - assert(EnableCoroutine, "pre-condition"); - Coroutine* coro = (Coroutine*)coroPtr; - Coroutine* target = (Coroutine*)targetPtr; - CoroutineListLocker cll(coro->thread()); - Coroutine::move(coro, target); -JVM_END - JVM_ENTRY (void, CoroutineSupport_markThreadCoroutine(JNIEnv* env, jclass klass, jlong coroPtr, jobject coroObj)) assert(EnableCoroutine, "pre-condition"); Coroutine* coro = (Coroutine*)coroPtr; @@ -999,14 +991,16 @@ JVM_ENTRY(jboolean, CoroutineSupport_stealCoroutine(JNIEnv* env, jclass klass, j assert(coro->thread() != thread, "steal from self"); assert(coro->state() != Coroutine::_current, "running"); - CoroutineListLocker cll(coro->thread(), thread); - // check if the source thread and the target thread has been scanned or not - // only if hold the CoroutineListLocker, and has the same scanned state, then we can steal - if (coro->thread()->nmethod_traversals() != thread->nmethod_traversals()) { - return false; + { + CoroutineListLocker cll(coro->thread(), thread); + // check if the source thread and the target thread has been scanned or not + // only if hold the CoroutineListLocker, and has the same scanned state, then we can steal + if (coro->thread()->nmethod_traversals() < thread->nmethod_traversals()) { + return false; + } + coro->remove_from_list(coro->thread()->coroutine_list()); + coro->insert_into_list(thread->coroutine_list()); } - coro->remove_from_list(coro->thread()->coroutine_list()); - coro->insert_into_list(thread->coroutine_list()); // change thread logic if (coro->last_handle_mark() != NULL) { coro->last_handle_mark()->change_thread_for_wisp(thread); @@ -1148,7 +1142,6 @@ JNINativeMethod coroutine_support_methods[] = { {CC"setWispBooted", CC "()V", FN_PTR(CoroutineSupport_setWispBooted)}, {CC"stealCoroutine", CC "(J)Z", FN_PTR(CoroutineSupport_stealCoroutine)}, {CC"getNextCoroutine", CC "(J)" COR, FN_PTR(CoroutineSupport_getNextCoroutine)}, - {CC"moveCoroutine", CC "(JJ)V", FN_PTR(CoroutineSupport_moveCoroutine)}, {CC"markThreadCoroutine", CC "(J" COBA ")V", FN_PTR(CoroutineSupport_markThreadCoroutine)}, {CC"getCoroutineStack", CC "(J)[" STE, FN_PTR(CoroutineSupport_getCoroutineStack)}, {CC"shouldThrowException0", CC "(J)Z", FN_PTR(CoroutineSupport_shouldThrowException0)}, diff --git a/src/hotspot/share/runtime/sweeper.cpp b/src/hotspot/share/runtime/sweeper.cpp index a87c08c428a..f011b4f964d 100644 --- a/src/hotspot/share/runtime/sweeper.cpp +++ b/src/hotspot/share/runtime/sweeper.cpp @@ -159,9 +159,6 @@ class NMethodMarkingClosure : public HandshakeClosure { void do_thread(Thread* thread) { if (thread->is_Java_thread() && ! thread->is_Code_cache_sweeper_thread()) { thread->as_Java_thread()->nmethods_do(_cl); - if (EnableCoroutine) { - thread->as_Java_thread()->set_nmethod_traversals(NMethodSweeper::traversal_count()); - } } } }; @@ -195,6 +192,10 @@ CodeBlobClosure* NMethodSweeper::prepare_mark_active_nmethods() { return &mark_activation_closure; } +CodeBlobClosure* NMethodSweeper::mark_active_closure() { + return &mark_activation_closure; +} + /** * This function triggers a VM operation that does stack scanning of active * methods. Stack scanning is mandatory for the sweeper to make progress. diff --git a/src/hotspot/share/runtime/sweeper.hpp b/src/hotspot/share/runtime/sweeper.hpp index 5f4e0ed7796..80e72e76ad8 100644 --- a/src/hotspot/share/runtime/sweeper.hpp +++ b/src/hotspot/share/runtime/sweeper.hpp @@ -112,6 +112,7 @@ class NMethodSweeper : public AllStatic { #endif static CodeBlobClosure* prepare_mark_active_nmethods(); + static CodeBlobClosure* mark_active_closure(); static void sweeper_loop(); static bool should_start_aggressive_sweep(); static void force_sweep(); diff --git a/src/hotspot/share/runtime/thread.cpp b/src/hotspot/share/runtime/thread.cpp index 0eb346f105f..ff11a49de05 100644 --- a/src/hotspot/share/runtime/thread.cpp +++ b/src/hotspot/share/runtime/thread.cpp @@ -106,6 +106,7 @@ #include "runtime/stackFrameStream.inline.hpp" #include "runtime/stackWatermarkSet.hpp" #include "runtime/statSampler.hpp" +#include "runtime/sweeper.hpp" #include "runtime/task.hpp" #include "runtime/thread.inline.hpp" #include "runtime/threadCritical.hpp" @@ -2179,6 +2180,11 @@ void JavaThread::nmethods_do(CodeBlobClosure* cf) { current->compiledMethods_do(cf); current = current->next(); } while (current != _coroutine_list); + + if (NMethodSweeper::mark_active_closure() == cf) { + // mark for current thread has been scanned. + set_nmethod_traversals(NMethodSweeper::traversal_count()); + } } else { if (jvmti_thread_state() != NULL) { jvmti_thread_state()->nmethods_do(cf); diff --git a/src/java.base/linux/classes/com/alibaba/wisp/engine/WispTask.java b/src/java.base/linux/classes/com/alibaba/wisp/engine/WispTask.java index b9b395d564c..07ea3b0355a 100644 --- a/src/java.base/linux/classes/com/alibaba/wisp/engine/WispTask.java +++ b/src/java.base/linux/classes/com/alibaba/wisp/engine/WispTask.java @@ -335,7 +335,7 @@ static void switchTo(WispTask current, WispTask next, boolean terminal) { // store load barrier is not necessary } if (terminal == true) { - current.carrier.thread.getCoroutineSupport().terminateCoroutine(next.ctx); + current.carrier.thread.getCoroutineSupport().terminateCoroutine(); // should never run here. assert false: "should not reach here"; } else { diff --git a/src/java.base/share/classes/java/dyn/CoroutineBase.java b/src/java.base/share/classes/java/dyn/CoroutineBase.java index c27c9414bd4..d95c744e216 100644 --- a/src/java.base/share/classes/java/dyn/CoroutineBase.java +++ b/src/java.base/share/classes/java/dyn/CoroutineBase.java @@ -35,8 +35,6 @@ public abstract class CoroutineBase { boolean finished = false; - boolean needsUnlock = false; - transient CoroutineSupport threadSupport; /** @@ -68,9 +66,6 @@ public abstract class CoroutineBase { private final void startInternal() { assert threadSupport.getThread() == SharedSecrets.getJavaLangAccess().currentThread0(); try { - // When we symmetricYieldTo a newly created coroutine, - // we'll expect the new coroutine release lock as soon as possible - threadSupport.beforeResume(this); run(); } catch (Throwable t) { if (!(t instanceof CoroutineExitException)) { @@ -78,10 +73,7 @@ private final void startInternal() { } } finally { finished = true; - // threadSupport is fixed by steal() - threadSupport.beforeResume(this); - - threadSupport.terminateCoroutine(null); + threadSupport.terminateCoroutine(); } assert threadSupport.getThread() == SharedSecrets.getJavaLangAccess().currentThread0(); } diff --git a/src/java.base/share/classes/java/dyn/CoroutineSupport.java b/src/java.base/share/classes/java/dyn/CoroutineSupport.java index 90d59b59453..6dca7cd8758 100644 --- a/src/java.base/share/classes/java/dyn/CoroutineSupport.java +++ b/src/java.base/share/classes/java/dyn/CoroutineSupport.java @@ -31,7 +31,6 @@ import sun.reflect.generics.reflectiveObjects.NotImplementedException; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import com.alibaba.wisp.engine.WispTask; @@ -41,9 +40,6 @@ @Contended public class CoroutineSupport { - private static final boolean CHECK_LOCK = true; - private static final int SPIN_BACKOFF_LIMIT = 2 << 8; - private static AtomicInteger idGen = new AtomicInteger(); // The thread that this CoroutineSupport belongs to. There's only one CoroutineSupport per Thread @@ -54,12 +50,6 @@ public class CoroutineSupport { // The currently executing coroutine private Coroutine currentCoroutine; - // protect the coroutine support - // double link list of JavaThread->coroutine_list() is protected by CoroutineListLock - private volatile Thread lockOwner = null; - - private int lockRecursive; // volatile is not need - private final int id; private boolean terminated = false; @@ -92,12 +82,7 @@ public Coroutine threadCoroutine() { void addCoroutine(Coroutine coroutine, long stacksize) { assert currentCoroutine != null; - lock(); - try { - coroutine.nativeCoroutine = createCoroutine(coroutine, stacksize); - } finally { - unlock(); - } + coroutine.nativeCoroutine = createCoroutine(coroutine, stacksize); } Thread getThread() { @@ -132,7 +117,6 @@ public void drain() { throw new IllegalArgumentException("Cannot drain another threads CoroutineThreadSupport"); } - lock(); try { // drain all coroutines Coroutine next = null; @@ -148,9 +132,7 @@ public void drain() { } catch (Throwable t) { t.printStackTrace(); } finally { - assert lockOwner == thread && lockRecursive == 0; terminated = true; - unlock(); } } @@ -171,8 +153,6 @@ public void unsafeSymmetricYieldTo(Coroutine target) { final Coroutine current = currentCoroutine; currentCoroutine = target; switchTo(current, target); - //check if locked by exiting coroutine - beforeResume(current); } /** @@ -180,13 +160,9 @@ public void unsafeSymmetricYieldTo(Coroutine target) { * @param target coroutine */ public void symmetricYieldTo(Coroutine target) { - lock(); if (target.threadSupport != this) { - unlock(); return; } - moveCoroutine(currentCoroutine.nativeCoroutine, target.nativeCoroutine); - unlockLater(target); unsafeSymmetricYieldTo(target); } @@ -195,19 +171,11 @@ public void symmetricYieldTo(Coroutine target) { * @param target coroutine */ public void symmetricStopCoroutine(Coroutine target) { - Coroutine current; - lock(); - try { - if (target.threadSupport != this) { - unlock(); - return; - } - current = currentCoroutine; - currentCoroutine = target; - moveCoroutine(current.nativeCoroutine, target.nativeCoroutine); - } finally { - unlock(); + if (target.threadSupport != this) { + return; } + Coroutine current = currentCoroutine; + currentCoroutine = target; switchToAndExit(current, target); } @@ -220,31 +188,21 @@ void symmetricExitInternal(Coroutine coroutine) { assert coroutine.threadSupport == this; if (!testDisposableAndTryReleaseStack(coroutine.nativeCoroutine)) { - moveCoroutine(currentCoroutine.nativeCoroutine, coroutine.nativeCoroutine); - final Coroutine current = currentCoroutine; currentCoroutine = coroutine; switchToAndExit(current, coroutine); - beforeResume(current); } } /** * terminate current coroutine and yield forward - * @param target target */ - public void terminateCoroutine(Coroutine target) { + public void terminateCoroutine() { assert currentCoroutine != threadCoroutine : "cannot exit thread coroutine"; - lock(); Coroutine old = currentCoroutine; - Coroutine forward = target; - if (forward == null) { - forward = getNextCoroutine(old.nativeCoroutine); - } - assert forward == threadCoroutine : "switch to target must be thread coroutine"; + Coroutine forward = threadCoroutine; currentCoroutine = forward; - unlockLater(forward); switchToAndTerminate(old, forward); // should never run here. @@ -267,101 +225,19 @@ Coroutine.StealResult steal(Coroutine coroutine, boolean failOnContention) { return Coroutine.StealResult.SUCCESS; } - if (source.id < target.id) { // prevent dead lock - if (!source.lockInternal(failOnContention)) { - return Coroutine.StealResult.FAIL_BY_CONTENTION; - } - target.lock(); - } else { - target.lock(); - if (!source.lockInternal(failOnContention)) { - target.unlock(); - return Coroutine.StealResult.FAIL_BY_CONTENTION; - } + if (source.terminated || coroutine.finished || + coroutine.threadSupport != source || // already been stolen + source.currentCoroutine == coroutine) { + return Coroutine.StealResult.FAIL_BY_STATUS; } - - try { - if (source.terminated || coroutine.finished || - coroutine.threadSupport != source || // already been stolen - source.currentCoroutine == coroutine) { - return Coroutine.StealResult.FAIL_BY_STATUS; - } - if (!stealCoroutine(coroutine.nativeCoroutine)) { // native frame - return Coroutine.StealResult.FAIL_BY_NATIVE_FRAME; - } - coroutine.threadSupport = target; - } finally { - source.unlock(); - target.unlock(); + if (!stealCoroutine(coroutine.nativeCoroutine)) { // native frame + return Coroutine.StealResult.FAIL_BY_NATIVE_FRAME; } + coroutine.threadSupport = target; return Coroutine.StealResult.SUCCESS; } - /** - * Can not be stolen while executing this, because lock is held - */ - void beforeResume(CoroutineBase source) { - if (source.needsUnlock) { - source.needsUnlock = false; - source.threadSupport.unlock(); - } - } - - private void unlockLater(CoroutineBase next) { - if (CHECK_LOCK && next.needsUnlock) { - throw new InternalError("pending unlock"); - } - next.needsUnlock = true; - } - - private void lock() { - boolean success = lockInternal(false); - assert success; - } - - private boolean lockInternal(boolean tryingLock) { - final Thread th = SharedSecrets.getJavaLangAccess().currentThread0(); - if (lockOwner == th) { - lockRecursive++; - return true; - } - for (int spin = 1; ; ) { - if (lockOwner == null && LOCK_UPDATER.compareAndSet(this, null, th)) { - return true; - } - for (int i = 0; i < spin; ) { - i++; - } - if (spin == SPIN_BACKOFF_LIMIT) { - if (tryingLock) { - return false; - } - SharedSecrets.getJavaLangAccess().yield0(); // yield safepoint - } else { // back off - spin *= 2; - } - } - } - - private void unlock() { - if (CHECK_LOCK && SharedSecrets.getJavaLangAccess().currentThread0() != lockOwner) { - throw new InternalError("unlock from non-owner thread"); - } - if (lockRecursive > 0) { - lockRecursive--; - } else { - LOCK_UPDATER.lazySet(this, null); - } - } - - private static final AtomicReferenceFieldUpdater LOCK_UPDATER; - - static { - LOCK_UPDATER = AtomicReferenceFieldUpdater.newUpdater(CoroutineSupport.class, Thread.class, "lockOwner"); - } - - /** * @param coroutine the coroutine * @return whether the coroutine is current coroutine @@ -402,13 +278,6 @@ public CoroutineBase getCurrent() { */ private static native Coroutine getNextCoroutine(long coroPtr); - /** - * move coroPtr to targetPtr's next field in underlying hotspot coroutine list - * @param coroPtr current threadCoroutine - * @param targetPtr coroutine that is about to exit - */ - private static native void moveCoroutine(long coroPtr, long targetPtr); - /** * track hotspot couroutine with java coroutine. * @param coroPtr threadCoroutine in hotspot