From 304431ef76512290dcd2132478ff89eb7e9a1477 Mon Sep 17 00:00:00 2001 From: Lin Hu Date: Thu, 29 Sep 2022 21:05:49 -0400 Subject: [PATCH] Avoid double scan stacks in continuation for compact case When virtual thread mounts to J9VMThread, the GC might scan java stack of the related continuation twice (one from J9VMThread.currentContinuation during thread scanning, another one from the related live continuation Object). Scan twice the same java stack during compaction fix-up might cause wrong double fixup and we also might need to both scan for concurrent case(there is no issue for double scan in concurrent collectors). so we stop to scan the stack of the continuation Object, which is mounted to the J9VMThread for STW collectors. Signed-off-by: Lin Hu --- runtime/gc/gctable.c | 2 +- runtime/gc_base/gc_internal.h | 2 +- runtime/gc_base/modronapi.cpp | 3 +-- runtime/gc_base/modronapi.hpp | 2 +- .../gc_glue_java/CompactSchemeFixupObject.cpp | 6 ++++- runtime/gc_vlhgc/WriteOnceCompactor.cpp | 6 ++++- runtime/oti/VMHelpers.hpp | 25 +++++++++++++++++-- runtime/oti/j9nonbuilder.h | 3 ++- runtime/vm/BytecodeInterpreter.hpp | 2 +- runtime/vm/ContinuationHelpers.cpp | 3 ++- 10 files changed, 42 insertions(+), 12 deletions(-) diff --git a/runtime/gc/gctable.c b/runtime/gc/gctable.c index 14221201260..706a3c7e4ba 100644 --- a/runtime/gc/gctable.c +++ b/runtime/gc/gctable.c @@ -252,7 +252,7 @@ J9MemoryManagerFunctions MemoryManagerFunctions = { ownableSynchronizerObjectCreated, continuationObjectCreated, preMountContinuation, - postDismountContinuation, + postUnmountContinuation, j9gc_notifyGCOfClassReplacement, j9gc_get_jit_string_dedup_policy, j9gc_stringHashFn, diff --git a/runtime/gc_base/gc_internal.h b/runtime/gc_base/gc_internal.h index b8501c6b888..ff5e8689a15 100644 --- a/runtime/gc_base/gc_internal.h +++ b/runtime/gc_base/gc_internal.h @@ -269,7 +269,7 @@ extern J9_CFUNC UDATA ownableSynchronizerObjectCreated(J9VMThread *vmThread, j9o extern J9_CFUNC UDATA continuationObjectCreated(J9VMThread *vmThread, j9object_t object); extern J9_CFUNC void preMountContinuation(J9VMThread *vmThread, j9object_t object); -extern J9_CFUNC void postDismountContinuation(J9VMThread *vmThread, j9object_t object); +extern J9_CFUNC void postUnmountContinuation(J9VMThread *vmThread, j9object_t object); extern J9_CFUNC void j9gc_notifyGCOfClassReplacement(J9VMThread *vmThread, J9Class *originalClass, J9Class *replacementClass, UDATA isFastHCR); diff --git a/runtime/gc_base/modronapi.cpp b/runtime/gc_base/modronapi.cpp index 724cbc4fe44..f9231b59354 100644 --- a/runtime/gc_base/modronapi.cpp +++ b/runtime/gc_base/modronapi.cpp @@ -56,11 +56,10 @@ void preMountContinuation(J9VMThread *vmThread, j9object_t object) { /* need read barrier to handle concurrent scavenger and gcpolicy:metronome case */ - } void -postDismountContinuation(J9VMThread *vmThread, j9object_t object) +postUnmountContinuation(J9VMThread *vmThread, j9object_t object) { /* Conservatively assume that via mutations of stack slots (which are not subject to access barriers), * all post-write barriers have been triggered on this Continuation object, since it's been mounted. */ diff --git a/runtime/gc_base/modronapi.hpp b/runtime/gc_base/modronapi.hpp index 873fe2bb841..b4d0dc4a250 100644 --- a/runtime/gc_base/modronapi.hpp +++ b/runtime/gc_base/modronapi.hpp @@ -103,7 +103,7 @@ UDATA ownableSynchronizerObjectCreated(J9VMThread *vmThread, j9object_t object); UDATA continuationObjectCreated(J9VMThread *vmThread, j9object_t object); void preMountContinuation(J9VMThread *vmThread, j9object_t object); -void postDismountContinuation(J9VMThread *vmThread, j9object_t object); +void postUnmountContinuation(J9VMThread *vmThread, j9object_t object); /** * Called during class redefinition to notify the GC of replaced classes.In certain cases the GC needs to diff --git a/runtime/gc_glue_java/CompactSchemeFixupObject.cpp b/runtime/gc_glue_java/CompactSchemeFixupObject.cpp index aefc95a1ce5..29c0be9d276 100644 --- a/runtime/gc_glue_java/CompactSchemeFixupObject.cpp +++ b/runtime/gc_glue_java/CompactSchemeFixupObject.cpp @@ -73,7 +73,11 @@ MM_CompactSchemeFixupObject::fixupContinuationObject(MM_EnvironmentStandard *env fixupMixedObject(objectPtr); /* fixup Java Stacks in J9VMContinuation */ J9VMThread *currentThread = (J9VMThread *)env->getLanguageVMThread(); - if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr)) { + /* In sliding compaction we must fix slots exactly once. Since we will fixup stack slots of + * mounted Virtual threads later during root fixup, we will skip it during this heap fixup pass + * (hence passing true for scanOnlyUnmounted parameter). + */ + if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr, true)) { StackIteratorData4CompactSchemeFixupObject localData; localData.compactSchemeFixupObject = this; localData.env = env; diff --git a/runtime/gc_vlhgc/WriteOnceCompactor.cpp b/runtime/gc_vlhgc/WriteOnceCompactor.cpp index 4a191e9acb7..79efe859848 100644 --- a/runtime/gc_vlhgc/WriteOnceCompactor.cpp +++ b/runtime/gc_vlhgc/WriteOnceCompactor.cpp @@ -1237,7 +1237,11 @@ MM_WriteOnceCompactor::fixupContinuationObject(MM_EnvironmentVLHGC* env, J9Objec fixupMixedObject(env, objectPtr, cache); J9VMThread *currentThread = (J9VMThread *)env->getLanguageVMThread(); - if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr)) { + /* In sliding compaction we must fix slots exactly once. Since we will fixup stack slots of + * mounted Virtual threads later during root fixup, we will skip it during this heap fixup pass + * (hence passing true for scanOnlyUnmounted parameter). + */ + if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr, true)) { StackIteratorData4WriteOnceCompactor localData; localData.writeOnceCompactor = this; localData.env = env; diff --git a/runtime/oti/VMHelpers.hpp b/runtime/oti/VMHelpers.hpp index 72526cfc958..73237561a32 100644 --- a/runtime/oti/VMHelpers.hpp +++ b/runtime/oti/VMHelpers.hpp @@ -2057,13 +2057,34 @@ class VM_VMHelpers return rc; } +#if JAVA_SPEC_VERSION >= 19 + /** + * Check if the related J9VMContinuation is mounted to carrier thread + * @param[in] continuation the related J9VMContinuation + * @return true if it is mounted. + */ static VMINLINE bool - needScanStacksForContinuation(J9VMThread *vmThread, j9object_t continuationObject) { + isContinuationMounted(J9VMContinuation *continuation) + { + return (NULL != continuation->carrierThread); + } +#endif /* JAVA_SPEC_VERSION >= 19 */ + + /** + * Check if we need to scan the java stack for the Continuation Object + * @param[in] vmThread the current J9VMThread + * @param[in] continuationObject the continuation object + * @param[in] scanOnlyUnmounted if it is true, only scan unmounted continuation object, default is false + * @return true if we need to scan the java stack + */ + static VMINLINE bool + needScanStacksForContinuation(J9VMThread *vmThread, j9object_t continuationObject, bool scanOnlyUnmounted = false) + { bool needScan = false; #if JAVA_SPEC_VERSION >= 19 jboolean started = J9VMJDKINTERNALVMCONTINUATION_STARTED(vmThread, continuationObject); J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, continuationObject); - needScan = started && (NULL != continuation); + needScan = started && (NULL != continuation) && (!scanOnlyUnmounted || !isContinuationMounted(continuation)); #endif /* JAVA_SPEC_VERSION >= 19 */ return needScan; } diff --git a/runtime/oti/j9nonbuilder.h b/runtime/oti/j9nonbuilder.h index 49a6f7e8d8c..a738b2f476b 100644 --- a/runtime/oti/j9nonbuilder.h +++ b/runtime/oti/j9nonbuilder.h @@ -4500,7 +4500,7 @@ typedef struct J9MemoryManagerFunctions { UDATA ( *ownableSynchronizerObjectCreated)(struct J9VMThread *vmThread, j9object_t object) ; UDATA ( *continuationObjectCreated)(struct J9VMThread *vmThread, j9object_t object) ; void ( *preMountContinuation)(struct J9VMThread *vmThread, j9object_t object) ; - void ( *postDismountContinuation)(struct J9VMThread *vmThread, j9object_t object) ; + void ( *postUnmountContinuation)(struct J9VMThread *vmThread, j9object_t object) ; void ( *j9gc_notifyGCOfClassReplacement)(struct J9VMThread *vmThread, J9Class *originalClass, J9Class *replacementClass, UDATA isFastHCR) ; I_32 ( *j9gc_get_jit_string_dedup_policy)(struct J9JavaVM *javaVM) ; @@ -4998,6 +4998,7 @@ typedef struct J9VMContinuation { UDATA* j2iFrame; struct J9JITGPRSpillArea jitGPRs; struct J9VMEntryLocalStorage* oldEntryLocalStorage; + struct J9VMThread* carrierThread; } J9VMContinuation; #endif /* JAVA_SPEC_VERSION >= 19 */ diff --git a/runtime/vm/BytecodeInterpreter.hpp b/runtime/vm/BytecodeInterpreter.hpp index 423e7fb9334..d3b0c25a21b 100644 --- a/runtime/vm/BytecodeInterpreter.hpp +++ b/runtime/vm/BytecodeInterpreter.hpp @@ -5232,7 +5232,7 @@ class INTERPRETER_CLASS j9object_t continuationObject = J9VMJAVALANGTHREAD_CONT(_currentThread, _currentThread->carrierThreadObject); /* Notify GC of Continuation stack swap */ - _vm->memoryManagerFunctions->postDismountContinuation(_currentThread, continuationObject); + _vm->memoryManagerFunctions->postUnmountContinuation(_currentThread, continuationObject); VMStructHasBeenUpdated(REGISTER_ARGS); restoreInternalNativeStackFrame(REGISTER_ARGS); diff --git a/runtime/vm/ContinuationHelpers.cpp b/runtime/vm/ContinuationHelpers.cpp index 9fc8413659e..a2b84534c6d 100644 --- a/runtime/vm/ContinuationHelpers.cpp +++ b/runtime/vm/ContinuationHelpers.cpp @@ -98,6 +98,7 @@ enterContinuation(J9VMThread *currentThread, j9object_t continuationObject) Assert_VM_Null(currentThread->currentContinuation); VM_ContinuationHelpers::swapFieldsWithContinuation(currentThread, continuation); + continuation->carrierThread = currentThread; currentThread->currentContinuation = continuation; /* Reset counters which determine if the current continuation is pinned. */ @@ -142,7 +143,7 @@ yieldContinuation(J9VMThread *currentThread) Assert_VM_notNull(currentThread->currentContinuation); VM_ContinuationHelpers::swapFieldsWithContinuation(currentThread, continuation); - + continuation->carrierThread = NULL; currentThread->currentContinuation = NULL; /* We need a full fence here to preserve happens-before relationship on PPC and other weakly