Handle continuation mounting case during Metronome GC #16183

LinHu2016 · 2022-10-24T20:10:19Z

We use scanning the continuation object to trigger related preReadBarrier processing in order to handle concurrent case during Metronome GC, but for root scanning and tracing phase, the mounting carrier thread(mutator thread) has not been initialized for markObject. so update to use rememberObject instead of markObject for handling preCountinuationMount case.

depends on:#16157

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

LinHu2016 · 2022-10-25T14:46:36Z

@amicic please review the change, Thanks

amicic · 2022-10-25T17:38:51Z

runtime/gc_glue_java/MetronomeDelegate.cpp

+		return 0;
+	} else {
+		return scanMixedObject(env, objectPtr);
+	}


Let's do some cleanup. We've been calling this special method do/fixup/scanContinuationObject for various collectors, but the method was really only scanning associated java stack, not the continuation object itself. At some point you wanted to introduce a wrapper to make it more clean, but I think that just this whole method (family of methods) should be renamed to a relatively generic name like do/fixup/scanContinuationNativeSlots (or OffheapSlots). We could be more specific too, like scanContinuationJavaStack - up for discussion.

Specifically for RT, that method should not call scanMixedObject at all (like for other GCs, it does not create a scanner). Then,

for preMount, what is in mutator thread context, we can call it directly, without any other action and context related checks (if mutator or GC thread).

for object scan, in GC thread context, when we recognize this scan type, we call this method, and right after that we call scanMixedObject, and we are still free of any context related checks (similar to other GCs where we create scanner in a separate step following java stack scan)

Optionally, we could even have scanContinuationObject too, beside scanContinuationNativeSlots, that will be just a wrapper around pair of: scanContinuationNativeSlots and the call to scanMixedObject (or construct a scanner). That is to keep the code more uniform with some other specialized objects that need extra action (like scanReferenceObject)

amicic · 2022-10-25T22:41:04Z

runtime/gc_vlhgc/CopyForwardScheme.cpp

+}
+
+MMINLINE void
+MM_CopyForwardScheme::scanContinuationNativeSlots(MM_EnvironmentVLHGC *env, MM_AllocationContextTarok *reservingContext, J9Object *objectPtr, ScanReason reason)


this is a duplicate definition (got one already from renaming scanContinuationObjectSlots)

amicic · 2022-10-25T22:43:07Z

runtime/gc_vlhgc/CopyForwardScheme.cpp

@@ -2337,6 +2336,17 @@ MM_CopyForwardScheme::scanContinuationObjectSlots(MM_EnvironmentVLHGC *env, MM_A
 	}
 }

+MMINLINE void
+MM_CopyForwardScheme::scanContinuationObjectSlots(MM_EnvironmentVLHGC *env, MM_AllocationContextTarok *reservingContext, J9Object *objectPtr, ScanReason reason)


I understand the name is inferred from scanMixedObjectSlots (and I don't know why it's the only one it does not call it simply scanMixedObject), but I'd still rename this method to scanContinuationObject, so it matched other continuation methods.

At some point we can rename scanMixedObjectSlots to scanMixedObject, but out of scope of this change.

amicic · 2022-10-25T22:50:17Z

runtime/gc_glue_java/MetronomeDelegate.cpp

@@ -1629,15 +1629,22 @@ stackSlotIteratorForRealtimeGC(J9JavaVM *javaVM, J9Object **slotPtr, void *local
 	if (realtimeMarkingScheme->isHeapObject(object)) {
 		/* heap object - validate and mark */
 		Assert_MM_validStackSlot(MM_StackSlotValidator(0, object, stackLocation, walkState).validate(env));
-		realtimeMarkingScheme->markObject(env, object);
+		if (MUTATOR_THREAD == env->getThreadType()) {


It would be slightly more efficient to move this check before calling stack slot iterator (so doing it only once per whole stack, rather then doing it per each slot).
It would require to specialize the callbacks (stackSlotIteratorForRealtimeGCFromMutator/FromGC). But, for now, I'll assume these stacks are small, and prefer code sharing, instead.

amicic · 2022-10-25T22:55:03Z

runtime/gc_glue_java/MetronomeDelegate.hpp

-	void markLiveObjectsComplete(MM_EnvironmentRealtime *env);
+	void markLiveObjectsCompl
+#if defined(J9VM_GC_DYNAMIC_CLASS_UNLOADING)
+	MMINLINE voidete(MM_EnvironmentRealtime *env);


some mixup here

amicic · 2022-10-25T22:55:47Z

runtime/gc_glue_java/MetronomeDelegate.hpp

 	UDATA scanContinuationObject(MM_EnvironmentRealtime *env, J9Object *objectPtr);

-#if defined(J9VM_GC_DYNAMIC_CLASS_UNLOADING)
-	MMINLINE void


amicic · 2022-10-25T23:00:10Z

runtime/gc_glue_java/CompactSchemeFixupObject.cpp

+MM_CompactSchemeFixupObject::fixupContinuationObject(MM_EnvironmentStandard *env, omrobjectptr_t objectPtr)
+{
+	fixupContinuationNativeSlots(env, objectPtr);
+	fixupMixedObject(objectPtr);


Changed the order of these operations from the original order. Don't remember it's relevant, but just in case, better to mention it. I like this one anyway (more consistent with other similar spots)

amicic · 2022-10-25T23:03:21Z

runtime/gc_glue_java/HeapWalkerDelegate.cpp

@@ -37,7 +37,7 @@ MM_HeapWalkerDelegate::objectSlotsDo(OMR_VMThread *omrVMThread, omrobjectptr_t o
 	MM_EnvironmentBase *env = MM_EnvironmentBase::getEnvironment(omrVMThread);
 	switch(_objectModel->getScanType(objectPtr)) {
 	case GC_ObjectModel::SCAN_CONTINUATION_OBJECT:
-		doContinuationObject(env, objectPtr, function, userData);
+		scanContinuationNativeSlots(env, objectPtr, function, userData);


we should be consistent in naming with the caller. Either the caller should be objectSlotsScan or this should be doContinuationNativeSlots. The latter is probably less intrusive change (other Walker APIs also tend to use 'do')

amicic · 2022-10-25T23:07:32Z

runtime/gc_vlhgc/WriteOnceCompactor.cpp

+MM_WriteOnceCompactor::fixupContinuationObject(MM_EnvironmentVLHGC* env, J9Object *objectPtr, J9MM_FixupCache *cache)
+{
+	fixupMixedObject(env, objectPtr, cache);
+	fixupContinuationNativeSlots(env, objectPtr, cache);


Make this order consistent with the order in MM_CompactSchemeFixupObject

amicic · 2022-10-26T14:04:47Z

runtime/gc_glue_java/ScavengerDelegate.cpp

@@ -340,7 +340,7 @@ stackSlotIteratorForScavenge(J9JavaVM *javaVM, J9Object **slotPtr, void *localDa
 }

 bool
-MM_ScavengerDelegate::doContinuationObject(MM_EnvironmentStandard *env, omrobjectptr_t objectPtr, MM_ScavengeScanReason reason)
+MM_ScavengerDelegate::doContinuationNativeSlots(MM_EnvironmentStandard *env, omrobjectptr_t objectPtr, MM_ScavengeScanReason reason)


This should probably be 'scan' (or maybe 'scavenge' - there is mixed bag of these 2 terms already). 'Do' makes the least sense.

amicic · 2022-10-26T14:08:39Z

runtime/gc_glue_java/CompactSchemeFixupObject.cpp

+void
+MM_CompactSchemeFixupObject::fixupContinuationObject(MM_EnvironmentStandard *env, omrobjectptr_t objectPtr)
+{
+	/* fixup continuation java stack first and then fixup the Object itself, fixup order shouldn't be matter. */


remove 'be' from 'shouldn't be matter'

amicic · 2022-10-26T14:16:48Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

@@ -1045,7 +1045,7 @@ MM_StandardAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t
 		/* concurrent scavenger in progress */
 		MM_EnvironmentStandard *env = (MM_EnvironmentStandard *) MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);


there is MM_EnvironmentStandard::getEnvironment, to do this in one step

amicic · 2022-10-26T14:17:38Z

runtime/gc_realtime/RealtimeAccessBarrier.cpp

@@ -955,7 +955,7 @@ MM_RealtimeAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t
 {
 	MM_EnvironmentRealtime *env = (MM_EnvironmentRealtime *) MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);


there is MM_EnvironmentRealtime::getEnvironment to do this in one step

We use scanning the continuation object to trigger related preReadBarrier processing in order to handle concurrent case during Metronome GC, but for root scanning and tracing phase, the mounting carrier thread(mutator thread) has not been initialized for markObject. so update to use rememberObject instead of markObject for handling preCountinuationMount case. Signed-off-by: Lin Hu <linhu@ca.ibm.com>

amicic · 2022-10-26T14:27:20Z

looks good, let's do a round of testing

amicic · 2022-10-26T14:27:37Z

jenkins compile win jdk8

amicic · 2022-10-26T14:27:49Z

jenkins compile aix jdk19

amicic · 2022-10-26T15:26:49Z

jenkins test sanity win jdk19

amicic · 2022-10-26T15:27:29Z

jenkins test sanity.system aix jdk19

amicic reviewed Oct 25, 2022

View reviewed changes

LinHu2016 force-pushed the GC_Loom_Metronome branch from b71040c to b6636d9 Compare October 25, 2022 21:17

amicic reviewed Oct 25, 2022

View reviewed changes

LinHu2016 force-pushed the GC_Loom_Metronome branch 2 times, most recently from f56bed0 to f6f73e5 Compare October 26, 2022 12:53

amicic reviewed Oct 26, 2022

View reviewed changes

LinHu2016 force-pushed the GC_Loom_Metronome branch from f6f73e5 to 3acf827 Compare October 26, 2022 14:11

amicic reviewed Oct 26, 2022

View reviewed changes

LinHu2016 force-pushed the GC_Loom_Metronome branch from 3acf827 to 91b9f1a Compare October 26, 2022 14:22

amicic merged commit 3a1ff46 into eclipse-openj9:master Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle continuation mounting case during Metronome GC #16183

Handle continuation mounting case during Metronome GC #16183

LinHu2016 commented Oct 24, 2022

LinHu2016 commented Oct 25, 2022

amicic Oct 25, 2022 •

edited

Loading

amicic Oct 25, 2022 •

edited

Loading

amicic Oct 25, 2022

amicic Oct 25, 2022 •

edited

Loading

amicic Oct 25, 2022

amicic Oct 25, 2022

amicic Oct 25, 2022

amicic Oct 25, 2022 •

edited

Loading

amicic Oct 25, 2022

amicic Oct 25, 2022

amicic Oct 26, 2022

amicic Oct 26, 2022

amicic Oct 26, 2022

amicic Oct 26, 2022

amicic commented Oct 26, 2022

amicic commented Oct 26, 2022

amicic commented Oct 26, 2022

amicic commented Oct 26, 2022

amicic commented Oct 26, 2022

		@@ -1045,7 +1045,7 @@ MM_StandardAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t
		/* concurrent scavenger in progress */
		MM_EnvironmentStandard env = (MM_EnvironmentStandard ) MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);

		@@ -955,7 +955,7 @@ MM_RealtimeAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t
		{
		MM_EnvironmentRealtime env = (MM_EnvironmentRealtime ) MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);

Handle continuation mounting case during Metronome GC #16183

Handle continuation mounting case during Metronome GC #16183

Conversation

LinHu2016 commented Oct 24, 2022

LinHu2016 commented Oct 25, 2022

amicic Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

amicic Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Oct 25, 2022 • 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 Oct 25, 2022 • 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 commented Oct 26, 2022

amicic commented Oct 26, 2022

amicic commented Oct 26, 2022

amicic commented Oct 26, 2022

amicic commented Oct 26, 2022

amicic Oct 25, 2022 •

edited

Loading

amicic Oct 25, 2022 •

edited

Loading

amicic Oct 25, 2022 •

edited

Loading

amicic Oct 25, 2022 •

edited

Loading