Fix missing concurrent continuation scanning issue #16614

LinHu2016 · 2023-01-26T22:04:28Z

The issue is caused by concurrent scavenger scanning and concurrent
marking scanning overlap for the same continuation Object, during
concurrent continuation scanning, the current synchronization control
would block the continuation mounting and ignore another concurrent
scanning, but the concurrent scavenger scanning and concurrent marking
are irrelevant, ignore another could cause missing scanning and the
related "live" object is recycled.

updated J9VMContinuation->state, use two low bits for recording
concurrentScanning(bit0 for concurrentScanningLocal and bit1 for
concurrentScanningGlobal) instead of only one low bit.
pass flag isConcurrentGC and flag isGlobalGC for
GC_VMThreadStackSlotIterator::scanSlots().
handle J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL and
J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_GLOBAL independently
only both J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL and
J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_GLOBAL bits has been cleared
we can notify blocked the continuation mounting thread.

fix:#16591

Signed-off-by: Lin Hu linhu@ca.ibm.com

amicic · 2023-01-31T19:46:42Z

runtime/oti/j9consts.h

@@ -492,8 +492,10 @@ extern "C" {
 #define J9_GC_MARK_MAP_LOG_SIZEOF_UDATA 0x5
 #define J9_GC_MARK_MAP_UDATA_MASK 0x1F
 #endif /* J9VM_ENV_DATA64 */
-#define J9_GC_CONTINUATION_STATE_INITIAL 0
-#define J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN 0x1
+#define J9_GC_CONTINUATION_STATE_CONCURRENT_NONE 0


_CONCURRENT_SCAN_NONE

amicic · 2023-01-31T21:48:27Z

runtime/oti/VMHelpers.hpp

+		uintptr_t state;
+		uintptr_t retState;
+		do {
+			state = continuation->state & (J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_ALL & (~checkConcurrentState));


Let's introduce a small helper, that could help with easier code understanding. Could be used here twice, and in 'exit' method, too.

/**

extract concurrent state of local GC, if this GC is global, and vice-versa

extract concurrent state of global GC, if this GC is local
*/

UDATA complementGCConcurrentState(UDATA continuationState, UDATA thisGCConcurrentState) {

return continuationState & (J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_ALL & ~thisGCConcurentState);

}

amicic · 2023-02-01T21:57:41Z

runtime/gc_structs/VMThreadStackSlotIterator.hpp

@@ -67,7 +67,8 @@ class GC_VMThreadStackSlotIterator
 			J9MODRON_OSLOTITERATOR *oSlotIterator,
 			bool includeStackFrameClassReferences,
 			bool trackVisibleFrameDepth,
-			bool syncWithContinuationMounting = false);
+			bool syncWithContinuationMounting = false,


small naming cleanup: let's rename this to isConcurrentGC (here and anywhere else this flag is used)

then both isConcurrentGC and isGlobalGC represent the facts as known by caller, without assuming how the callee will use them

amicic · 2023-02-01T21:58:21Z

runtime/gc_vlhgc/GlobalMarkingScheme.cpp

@@ -802,10 +802,10 @@ MM_GlobalMarkingScheme::scanContinuationNativeSlots(MM_EnvironmentVLHGC *env, J9
 		stackFrameClassWalkNeeded = isDynamicClassUnloadingEnabled();
 #endif /* J9VM_GC_DYNAMIC_CLASS_UNLOADING */

+
 		/* In STW GC there are no racing carrier threads doing mount and no need for the synchronization. */
 		bool syncWithContinuationMounting = (MM_VLHGCIncrementStats::mark_concurrent == static_cast<MM_CycleStateVLHGC*>(env->_cycleState)->_vlhgcIncrementStats._globalMarkIncrementType);


as mentioned earlier, let's rename this to isConcurrentGC

amicic · 2023-02-01T22:00:21Z

runtime/gc_vlhgc/GlobalMarkingScheme.cpp

@@ -792,7 +792,7 @@ void
 MM_GlobalMarkingScheme::scanContinuationNativeSlots(MM_EnvironmentVLHGC *env, J9Object *objectPtr, ScanReason reason)
 {
 	J9VMThread *currentThread = (J9VMThread *)env->getLanguageVMThread();
-	if (MM_GCExtensions::needScanStacksForContinuationObject(currentThread, objectPtr)) {
+	if (MM_GCExtensions::needScanStacksForContinuationObject(currentThread, objectPtr, true)) {


let's introduce a line
const bool isGlobalGC = true;
and instead of passing 'true' pass isGlobalGC at the 2 spots
that will help with undestanding what these flags we pass are about

update it for all other methods that use GC_VMThreadStackSlotIterator::scanSlots()

amicic · 2023-02-01T22:07:09Z

let's fix other caller's of GC_VMThreadStackSlotIterator::scanSlots() to not rely on default isGlobalGC = true, where really that's not the case (CopyForward for example).
WriteOnesCompactor might be a tricky since it's used for both, so you may need to query cycle state to figure out if it's local or global

amicic · 2023-02-01T22:09:21Z

runtime/oti/VMHelpers.hpp

+		uintptr_t concurrentState = J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL;
+		 if (isGlobalGC) {
+			 concurrentState = J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_GLOBAL;
+		 }


there is an extra emty space of indentation in 3 lines of the 'if' block

amicic · 2023-02-01T22:12:35Z

runtime/oti/VMHelpers.hpp

+	static VMINLINE bool
+	isConcurrentlyScannedFromContinuationState(uintptr_t continuationState, bool isGlobalGC)
+	{
+		uintptr_t concurrentState = J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL;


it's really a mask rather then state, so rename it to concurrentGCMask

amicic · 2023-02-01T22:13:56Z

runtime/oti/j9consts.h

+#define J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_NONE 0
+#define J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL  0x1
+#define J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_GLOBAL 0x2
+#define J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_ALL  	(J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL | J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_GLOBAL)


I prefer either _ANY or _MASK, rather than _ALL

amicic · 2023-02-01T22:18:30Z

runtime/oti/VMHelpers.hpp

+		uintptr_t state;
+		uintptr_t retState;
+		do {
+			state = continuation->state & getGCConcurrentState(!isGlobalGC);


let's give this state a more specific name (complementGCConcurrentState?) and provide a comment above the line:
/* preserve the concurrent GC state for the other type of GC */

amicic · 2023-02-01T22:19:31Z

runtime/oti/VMHelpers.hpp

+		uintptr_t retState;
+		do {
+			state = continuation->state & getGCConcurrentState(!isGlobalGC);
+			retState = VM_AtomicSupport::lockCompareExchange(&continuation->state, state, state | getGCConcurrentState(isGlobalGC));


rename to returnedState

amicic · 2023-02-01T22:22:27Z

runtime/oti/VMHelpers.hpp

+		do {
+			state = continuation->state & getGCConcurrentState(!isGlobalGC);
+			retState = VM_AtomicSupport::lockCompareExchange(&continuation->state, state, state | getGCConcurrentState(isGlobalGC));
+		} while (state != (retState & state));


a comment above this line would be good:
if the other GC happened to change its concurrentGC state since us taking a snapshot of their state, we'll have to retry

amicic · 2023-02-01T22:31:31Z

runtime/oti/VMHelpers.hpp

+	{
+		/* clear CONCURRENTSCANNING flag bit0:LocalConcurrentScanning /bit1:GlobalConcurrentScanning */
+		uintptr_t oldContinuationState = VM_AtomicSupport::bitAnd(&continuation->state, ~getGCConcurrentState(isGlobalGC));
+		if (!(oldContinuationState & getGCConcurrentState(!isGlobalGC))) {


store this into complementGCConcurrentState

amicic · 2023-02-02T19:13:17Z

runtime/oti/VMHelpers.hpp

+	    if (isGlobalGC) {
+	        return J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_GLOBAL);
+	    } else {
+	        return J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL);


spaces are used for indentation, while elsewhere tabs are used

amicic · 2023-02-02T19:19:42Z

runtime/gc_glue_java/HeapWalkerDelegate.cpp

@@ -60,14 +60,16 @@ MM_HeapWalkerDelegate::doContinuationNativeSlots(MM_EnvironmentBase *env, omrobj
 {
 	J9VMThread *currentThread = (J9VMThread *)env->getLanguageVMThread();

-	if (MM_GCExtensions::needScanStacksForContinuationObject(currentThread, objectPtr)) {
+	const bool isGlobalGC = false;


This is weird one, since Heap walker is not a GC, to start with.
Either false or true is probably good, but since this it's actually (so far) only instantiated by ParallelGlobalGC and used for whole heap walk, it'd actually mark it as global.
If in future it's used from different GCs, then we would have to resolve this in run-time - similar how we do it for WriteOnceCompactor.

seems like you forgot to flip this to true

this has not been fixed yet

amicic · 2023-02-02T19:33:13Z

runtime/gc_glue_java/MetronomeDelegate.cpp

-
-		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForRealtimeGC, stackFrameClassWalkNeeded, false, syncWithContinuationMounting);
+		bool isConcurrentGC = _realtimeGC->isCollectorConcurrentTracing();
+		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForRealtimeGC, stackFrameClassWalkNeeded, false, isConcurrentGC, isGlobalGC);


sometimes you have an empty line between the setting concurrent flag and this line, and sometimes not
let's be consistent, and have an empty line (here and check other spots, too)

amicic · 2023-02-02T19:38:53Z

runtime/oti/VMHelpers.hpp

+		return isContinuationMounted(continuation) || isConcurrentlyScannedFromContinuationState(continuation->state, isGlobalGC);
+	}
+
+	static VMINLINE uintptr_t getGCConcurrentState(bool isGlobalGC) {


you could rename it to getConcurrentGCMask, but I'll leave it up to you, if you like it

either way, you could use it at one more spot (where I got a motivation):
sConcurrentlyScannedFromContinuationState(uintptr_t continuationState, bool isGlobalGC)

amicic · 2023-02-02T19:43:15Z

runtime/oti/VMHelpers.hpp

+			returnedState = VM_AtomicSupport::lockCompareExchange(&continuation->state, complementGCConcurrentState, complementGCConcurrentState | getGCConcurrentState(isGlobalGC));
+		/* if the other GC happened to change its concurrentGC state since us taking a snapshot of their state, we'll have to retry */
+		} while (complementGCConcurrentState != (returnedState & complementGCConcurrentState));
+


add a comment:
if returned state does not contain carrier ID, return that we won

amicic · 2023-02-02T19:47:09Z

runtime/oti/VMHelpers.hpp

+			J9VMThread *carrierThread = getCarrierThreadFromContinuationState(oldContinuationState);
+			if (NULL != carrierThread) {
+				omrthread_monitor_enter(carrierThread->publicFlagsMutex);
+				/*  notify the waiting carrierThread that we just finished scanning, and it can proceed with mounting. */


expand this comment:

notify the waiting carrierThread that we just finished scanning and we were the only/last GC to scan it, so that it can proceed with mounting

amicic · 2023-02-02T19:50:40Z

runtime/oti/VMHelpers.hpp

 	{
-		return J9_GC_CONTINUATION_STATE_INITIAL == VM_AtomicSupport::lockCompareExchange(&continuation->state, J9_GC_CONTINUATION_STATE_INITIAL, J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN);
+		uintptr_t complementGCConcurrentState;
+		uintptr_t returnedState;


to comply with coding standards let's initialize these 2 with _NONE

amicic · 2023-02-02T19:52:40Z

runtime/oti/VMHelpers.hpp

-		if (syncWithContinuationMounting && (NULL != continuation)) {
-			if (!tryWinningConcurrentGCScan(continuation)) {
+		if (isConcurrentGC && (NULL != continuation)) {
+			if (!tryWinningConcurrentGCScan(continuation, isGlobalGC)) {
 				/* If continuation is mounted or already being scanned by another GC thread, we do nothing */


adjust this comment:

if continuation is mounted or already being scanned by another GC thread of the same GC type, we do nothing

amicic · 2023-02-02T20:58:56Z

runtime/gc_glue_java/ScavengerDelegate.cpp

 		StackIteratorData4Scavenge localData;
 		localData.scavengerDelegate = this;
 		localData.env = env;
 		localData.reason = reason;
 		localData.shouldRemember = &shouldRemember;
 		/* In STW GC there are no racing carrier threads doing mount and no need for the synchronization. */
-		bool  syncWithContinuationMounting = _extensions->isConcurrentScavengerInProgress();
+		bool  isConcurrentGC = _extensions->isConcurrentScavengerInProgress();


extra space (or indentation) between these 2: bool isConcurrentGC

amicic · 2023-02-02T21:03:25Z

runtime/oti/VMHelpers.hpp

@@ -2053,13 +2053,29 @@ class VM_VMHelpers
 	static VMINLINE J9VMThread *
 	getCarrierThreadFromContinuationState(uintptr_t continuationState)


move this method below next to isContinuationMounted - they are related/similar

amicic · 2023-02-02T21:04:51Z

runtime/oti/VMHelpers.hpp

+		return isContinuationMounted(continuation) || isConcurrentlyScannedFromContinuationState(continuation->state, isGlobalGC);
+	}
+
+	static VMINLINE uintptr_t getConcurrentGCMask(bool isGlobalGC) {


move this method up before isConcurrentlyScannedFromContinuationState that uses it

amicic · 2023-02-02T22:05:16Z

runtime/oti/VMHelpers.hpp

-	getCarrierThreadFromContinuationState(uintptr_t continuationState)
-	{
-		return (J9VMThread *)(continuationState & (~(uintptr_t)J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN));
+	static VMINLINE uintptr_t getConcurrentGCMask(bool isGlobalGC) {


move open curly bracket into the next line

amicic · 2023-02-03T16:01:01Z

runtime/oti/VMHelpers.hpp

 	static VMINLINE bool
 	isConcurrentlyScannedFromContinuationState(uintptr_t continuationState)
 	{
-		return J9_ARE_ANY_BITS_SET(continuationState, J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN);
+		return J9_ARE_ANY_BITS_SET(continuationState, J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_ANY);
+


remove empty line

amicic · 2023-02-03T16:01:10Z

runtime/oti/VMHelpers.hpp

+	{
+		uintptr_t concurrentGCMask = getConcurrentGCMask(isGlobalGC);
+		return J9_ARE_ANY_BITS_SET(continuationState, concurrentGCMask);
+


remove empty line

amicic · 2023-02-03T16:03:07Z

@tajila pretty much all GC code, but formally a couple of files are VM, so if you want to have a quick look...

tajila · 2023-02-03T16:15:48Z

Changes look good, but I think the exitConcurrentGCScan and related APIs should be moved to continuationhelpers.hpp. Doesn't need to be done in this PR

amicic · 2023-02-03T16:19:40Z

agree, it's been suggested already that all/most of continuation specific code moves out of this generic VMHelpers, and has been attempted, but apparently there were some compile dependencies that were not trivial to untangle
should try again, eventually

amicic · 2023-02-03T18:44:49Z

runtime/gc_glue_java/HeapWalkerDelegate.cpp

 		StackIteratorData4HeapWalker localData;
 		localData.heapWalker = _heapWalker;
 		localData.env = env;
 		localData.fromObject = objectPtr;
 		localData.function = function;
 		localData.userData = userData;
 		/* so far there is no case we need ClassWalk for heapwalker, so we set stackFrameClassWalkNeeded = false */
-		GC_VMThreadStackSlotIterator::scanSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForHeapWalker, false, false);
+		const bool isConcurrentGC = true;


this is not correct, it should be isGlobalGC that should be set to true

The issue is caused by concurrent scavenger scanning and concurrent marking scanning overlap for the same continuation Object, during concurrent continuation scanning, the current synchronization control would block the continuation mounting and ignore another concurrent scanning, but the concurrent scavenger scanning and concurrent marking are irrelevant, ignore another could cause missing scanning and the related "live" object is recycled. - updated J9VMContinuation->state, use two low bits for recording concurrentScanning(bit0 for concurrentScanningLocal and bit1 for concurrentScanningGlobal) instead of only one low bit. - pass flag isConcurrentGC and flag isGlobalGC for GC_VMThreadStackSlotIterator::scanSlots(). - handle J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL and J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_GLOBAL independently - only both J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_LOCAL and J9_GC_CONTINUATION_STATE_CONCURRENT_SCAN_GLOBAL bits has been cleared we can notify blocked the continuation mounting thread. Signed-off-by: Lin Hu <linhu@ca.ibm.com>

amicic · 2023-02-03T18:52:03Z

Jenkins test sanity aix jdk19

amicic · 2023-02-03T18:52:22Z

Jenkins compile win jdk11

amicic · 2023-02-03T23:16:29Z

Jenkins test sanity plinux jdk19

amicic · 2023-02-03T23:19:14Z

Jenkins test sanity aix jdk19

dmitripivkine added comp:gc project:loom Used to track Project Loom related work jdk19 labels Jan 26, 2023

dmitripivkine added this to the Java 19 0.37 milestone Jan 26, 2023

LinHu2016 force-pushed the GC_Loom_update_7 branch from ad77831 to 88c4106 Compare January 31, 2023 15:53

amicic reviewed Jan 31, 2023

View reviewed changes

LinHu2016 force-pushed the GC_Loom_update_7 branch from 88c4106 to 1cb453d Compare January 31, 2023 21:09

amicic reviewed Jan 31, 2023

View reviewed changes

LinHu2016 force-pushed the GC_Loom_update_7 branch from 1cb453d to 592bdd8 Compare February 1, 2023 21:21

amicic reviewed Feb 1, 2023

View reviewed changes

LinHu2016 force-pushed the GC_Loom_update_7 branch 2 times, most recently from 2fbc065 to 1d3bbe0 Compare February 2, 2023 18:59

amicic reviewed Feb 2, 2023

View reviewed changes

LinHu2016 force-pushed the GC_Loom_update_7 branch from 1d3bbe0 to 1f0aba4 Compare February 2, 2023 20:49

amicic reviewed Feb 2, 2023

View reviewed changes

LinHu2016 force-pushed the GC_Loom_update_7 branch from 1f0aba4 to 8f4a858 Compare February 2, 2023 21:08

amicic reviewed Feb 2, 2023

View reviewed changes

LinHu2016 force-pushed the GC_Loom_update_7 branch 2 times, most recently from a52672e to e32bd04 Compare February 3, 2023 15:58

amicic reviewed Feb 3, 2023

View reviewed changes

tajila approved these changes Feb 3, 2023

View reviewed changes

LinHu2016 force-pushed the GC_Loom_update_7 branch from e32bd04 to 95a0ff2 Compare February 3, 2023 18:23

amicic reviewed Feb 3, 2023

View reviewed changes

LinHu2016 force-pushed the GC_Loom_update_7 branch from 95a0ff2 to 13ebc10 Compare February 3, 2023 18:48

amicic merged commit af44f3e into eclipse-openj9:master Feb 4, 2023

amicic self-requested a review February 6, 2023 18:07

		@@ -2053,13 +2053,29 @@ class VM_VMHelpers
		static VMINLINE J9VMThread *
		getCarrierThreadFromContinuationState(uintptr_t continuationState)

Fix missing concurrent continuation scanning issue #16614

Fix missing concurrent continuation scanning issue #16614

Conversation

LinHu2016 commented Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

amicic commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic commented Feb 3, 2023

tajila commented Feb 3, 2023

amicic commented Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

amicic commented Feb 3, 2023

amicic commented Feb 3, 2023

amicic commented Feb 3, 2023

amicic commented Feb 3, 2023

LinHu2016 commented Jan 26, 2023 •

edited

Loading

amicic Feb 1, 2023 •

edited

Loading

amicic Feb 1, 2023 •

edited

Loading

amicic Feb 2, 2023 •

edited

Loading

amicic commented Feb 3, 2023 •

edited

Loading