Handle Continuation scan during Concurrent Scavenger and Metronome #16106

LinHu2016 · 2022-10-14T15:15:35Z

For CS and Metronome, due to synchronous nature of their barriers (they read the value of the slot and act on it immediately, for example they may evacuate object for CS and mark object for Metronome), We fully scan java stacks of related continuation during the virtual thread mount.

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

LinHu2016 · 2022-10-14T15:16:21Z

@amicic @dmitripivkine please review the change, Thanks

amicic · 2022-10-14T15:23:27Z

runtime/gc_base/modronapi.cpp

+ if ((J9_GC_POLICY_METRONOME == vmThread->javaVM->gcPolicy) && (extensions->realtimeGC->isCollectorConcurrentTracing())) {
+ /* metronome in progress */
+ extensions->realtimeGC->getRealtimeDelegate()->scanContinuationJavaStack((MM_EnvironmentRealtime *)env, object);
+ }


Let's try to 'repackage' this into ObjectAccessBarrier: have a virtual preMountContinuation that Standard and Realtime subclass will override. It's somewhat cleaner (since we don't have to query GC policy).

amicic · 2022-10-14T15:27:21Z

Noticed in existing code:

postUnmountContinuation(J9VMThread *vmThread, j9object_t object)
{
      J9WriteBarrierBatch(vmThread, object); <<< should be able to call directly

amicic · 2022-10-14T17:03:30Z

Noticed in existing code:

postUnmountContinuation(J9VMThread *vmThread, j9object_t object)
{
      J9WriteBarrierBatch(vmThread, object); <<< should be able to call directly

Since I suggested to introduce preMountContinuation in OAB, for symmetry we should do that for postUnmount, too. Then, just call postBatchObjectStore there (bypassing J9WriteBarrierBatch), for Standard and Balanced only and no need for an empty postBatchObjectStore for Realtime, that you had to do when using J9WriteBarrierBatch.

Chances are, we will have to do something in postUnmount for RT too (to compensate for missing double barrier), but let me think about it more. Unfortunately, this (need for double barrier) is very hard scenario to produce in real life workloads.

amicic · 2022-10-14T19:10:10Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

+ return true;
+}
+
+bool


Don't see value in bool ret type, for both pre/post - just make them void.

Don't know why postBatchObjectStore ever had return type bool, but seems like it was never used anywhere or ever returned anything else but true.

amicic · 2022-10-14T19:11:07Z

runtime/gc_glue_java/ScavengerDelegate.hpp

@@ -179,6 +179,12 @@ class MM_ScavengerDelegate : public MM_BaseNonVirtual {
 bool initialize(MM_EnvironmentBase *env);
 void tearDown(MM_EnvironmentBase *env);

+ void scanContinuationJavaStack(MM_EnvironmentStandard *env, omrobjectptr_t objectPtr)


don't see value in this wrapper

just make doContinuationObject public

amicic · 2022-10-14T19:15:05Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

+ MM_GCExtensions *extensions = MM_GCExtensions::getExtensions(vmThread->javaVM);
+ if (extensions->isConcurrentScavengerInProgress()) {
+ /* concurrent scavenger in progress */
+ extensions->scavenger->getDelegate()->scanContinuationJavaStack((MM_EnvironmentStandard *)(MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread)), contObject);


create env in a separate line for easier reading

let's cache scavenger, there is a dozen of places we use it in this class (I should have done it long ago)

amicic · 2022-10-14T19:18:14Z

runtime/gc_realtime/RealtimeAccessBarrier.cpp

+bool
+MM_RealtimeAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t contObject)
+{
+ if (_realtimeGC->isCollectorConcurrentTracing()) {


don't know if it's the same, but use isBarrierActive as it's already case in the rest of this file/class

amicic · 2022-10-14T19:22:47Z

runtime/gc_base/modronapi.cpp

+#include "MetronomeDelegate.hpp"
+#include "EnvironmentStandard.hpp"
+#include "Scavenger.hpp"
+#include "ScavengerDelegate.hpp"


most of these includes are not necessary anymore

amicic · 2022-10-14T19:24:45Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

+bool
+MM_StandardAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t contObject)
+{
+ MM_GCExtensions *extensions = MM_GCExtensions::getExtensions(vmThread->javaVM);


_extensions is already available

amicic · 2022-10-14T19:25:14Z

runtime/gc_realtime/RealtimeAccessBarrier.cpp

+MM_RealtimeAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t contObject)
+{
+ if (_realtimeGC->isCollectorConcurrentTracing()) {
+ _realtimeGC->getRealtimeDelegate()->scanContinuationJavaStack((MM_EnvironmentRealtime *)(MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread)), contObject);


create env in a separate line

amicic · 2022-10-14T19:32:40Z

runtime/gc_base/modronapi.cpp

 }

 void
 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. */


move this comment to at least Standard OAB

amicic · 2022-10-14T20:56:44Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

@@ -80,6 +80,8 @@ MM_StandardAccessBarrier::initialize(MM_EnvironmentBase *env)
 if (!_generationalAccessBarrierComponent.initialize(env)) {
 return false;
 }
+ _scavenger = MM_GCExtensions::getExtensions(env)->scavenger;
+ Assert_MM_true(NULL != _scavenger);


It's actually possible to be null. An obvious config is Optavgpause, but I think even Optthruput will create SAB (although barely use it, not so much for real barrier stuff, but for some java specific API)

amicic · 2022-10-14T21:07:28Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

@@ -901,7 +902,7 @@ MM_StandardAccessBarrier::preObjectRead(J9VMThread *vmThread, J9Object *srcObjec

 env->_scavengerStats._readObjectBarrierCopy += 1;
 if (GLOBAL_READ_BARRIR_STATS_UPDATE_THRESHOLD == env->_scavengerStats._readObjectBarrierCopy) {
- MM_AtomicOperations::addU64(&_extensions->scavengerStats._readObjectBarrierCopy, GLOBAL_READ_BARRIR_STATS_UPDATE_THRESHOLD);
+ MM_AtomicOperations::addU64(&_scavengerStats._readObjectBarrierCopy, GLOBAL_READ_BARRIR_STATS_UPDATE_THRESHOLD);


seems like editor's find&replace went too far

amicic · 2022-10-14T21:10:23Z

runtime/gc_realtime/RealtimeAccessBarrier.cpp

+void
+MM_RealtimeAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t contObject)
+{
+ MM_EnvironmentBase *env = MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);


convert it to MM_EnvironmentRealtime right here and then you don't need a cast later

amicic · 2022-10-14T21:14:03Z

runtime/gc_glue_java/ScavengerDelegate.hpp

@@ -179,6 +178,8 @@ class MM_ScavengerDelegate : public MM_BaseNonVirtual {
 bool initialize(MM_EnvironmentBase *env);
 void tearDown(MM_EnvironmentBase *env);

+ bool doContinuationObject(MM_EnvironmentStandard *env, omrobjectptr_t objectPtr, MM_ScavengeScanReason reason);


move it up above init/teardown, perhaps close to setShouldScavengeContinuationObjects?

BTW, we are somewhat inconsistent in naming. For example, MarkingDelegate calls this 'scan', not 'do'.... Not sure what is the right one, and if I want to fix it now (this is already growing too much).

amicic · 2022-10-14T21:29:58Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

+ /* concurrent scavenger in progress */
+ MM_EnvironmentStandard *env = (MM_EnvironmentStandard *) MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);
+ MM_ScavengeScanReason reason = SCAN_REASON_SCAVENGE;
+ _scavenger->getDelegate()->doContinuationObject(env, contObject, reason);


Do you know when thread is mounted first time if J9VMJDKINTERNALVMCONTINUATION_STARTED is returning true or false? Effectively no scan work should be done on first mount (obviously there is not stack yet), and we should realize that as early as possible. At least not to try to iterate stack, but ideally as early as in Interpreter in enterContinuation.
Nothing to do for this round of changes, though.

before first time enterContinuation(), J9VMJDKINTERNALVMCONTINUATION_STARTED return false, enterContinuation() will set it true.

before first time enterContinuation(), J9VMJDKINTERNALVMCONTINUATION_STARTED return false, enterContinuation() will set it true.

OK, thanks, that's what I expected. We will still waste time to enter the callback and barrier code, but at least we will not try to setup and enter stack scanning.

amicic · 2022-10-14T22:29:37Z

runtime/gc_modron_standard/StandardAccessBarrier.hpp

@@ -51,6 +51,7 @@ class MM_StandardAccessBarrier : public MM_ObjectAccessBarrier
 MM_GenerationalAccessBarrierComponent _generationalAccessBarrierComponent; /**< Generational Component of Access Barrier */
 #endif /* J9VM_GC_GENERATIONAL */
 MM_MarkingScheme *_markingScheme;
+ MM_Scavenger *_scavenger;


This should be under
OMR_GC_MODRON_SCAVENGER
or perhaps under
J9VM_GC_GENERATIONAL that is very similar although seems to be used more in context of barriers.

LinHu2016 · 2022-10-17T12:51:44Z

personal build: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/14480/

amicic · 2022-10-17T13:56:19Z

runtime/gc_base/modronapi.cpp

- * all post-write barriers have been triggered on this Continuation object, since it's been mounted. */
- vmThread->javaVM->memoryManagerFunctions->J9WriteBarrierBatch(vmThread, object);
+ MM_GCExtensions *extensions = MM_GCExtensions::getExtensions(vmThread->javaVM);
+ extensions->accessBarrier->postUnmountContinuation(vmThread, object);


they now probably fit better into accessBarrier.cpp

please move pre/postMountContinuation into accessBarrier.cpp

amicic · 2022-10-17T14:06:18Z

The changes look good, but I'll let @dmitripivkine review, before launching any testing.

amicic · 2022-10-17T14:21:03Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

+void
+MM_StandardAccessBarrier::preMountContinuation(J9VMThread *vmThread, j9object_t contObject)
+{
+#if defined(J9VM_GC_GENERATIONAL)


I'd put this under a more specific #ifdef OMR_GC_CONCURRENT_SCAVENGER

(declaration and initialization of _scavenger would still remain under J9VM_GC_GENERATIONAL)

amicic · 2022-10-17T14:21:58Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

@@ -265,10 +266,12 @@ MM_StandardAccessBarrier::postObjectStoreImpl(J9VMThread *vmThread, J9Object *ds
 {
 /* If the source object is NULL, there is no need for a write barrier. */
 if(NULL != srcObject) {
+#if defined(J9VM_GC_GENERATIONAL)


I'd put this under a more specific #ifdef OMR_GC_CONCURRENT_SCAVENGER

dmitripivkine · 2022-10-17T16:25:47Z

runtime/gc_modron_standard/StandardAccessBarrier.hpp

@@ -49,6 +49,7 @@ class MM_StandardAccessBarrier : public MM_ObjectAccessBarrier
 private:
 #if defined(J9VM_GC_GENERATIONAL)
 MM_GenerationalAccessBarrierComponent _generationalAccessBarrierComponent; /**< Generational Component of Access Barrier */
+ MM_Scavenger *_scavenger;
 #endif /* J9VM_GC_GENERATIONAL */
 MM_MarkingScheme *_markingScheme;


please add annotations to new _scavenger as well as existed _markingScheme variables

dmitripivkine · 2022-10-17T16:34:12Z

The changes look good, but I'll let @dmitripivkine review, before launching any testing.

looks reasonable to me

For CS and Metronome, due to synchronous nature of their barriers (they read the value of the slot and act on it immediately, for example they may evacuate object for CS and mark object for Metronome), We fully scan java stacks of related continuation during the virtual thread mount. Signed-off-by: Lin Hu <linhu@ca.ibm.com>

amicic · 2022-10-17T18:13:30Z

jenkins test sanity all jdk19

amicic · 2022-10-17T18:13:48Z

jenkins compile win,aix jdk8

dmitripivkine · 2022-10-18T13:22:56Z

jenkins test sanity all jdk19

amicic · 2022-10-18T13:23:43Z

3 out of 8 specs failed sanity testing but all due to infra issue (uploading artifacts)

dmitripivkine · 2022-10-18T20:00:22Z

Jenkins test sanity xLinux jdk19

dmitripivkine · 2022-10-18T20:00:44Z

Jenkins test sanity pLinux jdk19

dmitripivkine · 2022-10-18T20:17:29Z

Jenkins test sanity x86-64_mac jdk19

dmitripivkine · 2022-10-18T20:53:54Z

Failure in AIX build is unrelated most likely. Opened new issue #16121

dmitripivkine · 2022-10-18T20:59:35Z

Jenkins test sanity aix jdk19

dmitripivkine · 2022-10-19T03:26:03Z

xLinux build failures related to infra, failures in AIX build looks like bad machine

dmitripivkine · 2022-10-19T13:13:41Z

I think reasons for failure are unrelated, merging this item regardless

LinHu2016 force-pushed the supportConcurrentScavenger branch from 1efe539 to 58700a6 Compare October 14, 2022 15:17

amicic reviewed Oct 14, 2022

View reviewed changes

amicic added comp:gc project:loom Used to track Project Loom related work labels Oct 14, 2022

LinHu2016 force-pushed the supportConcurrentScavenger branch from 58700a6 to f7d3b28 Compare October 14, 2022 18:40

amicic reviewed Oct 14, 2022

View reviewed changes

LinHu2016 force-pushed the supportConcurrentScavenger branch 2 times, most recently from 7e1db01 to ac65f97 Compare October 14, 2022 20:43

amicic reviewed Oct 14, 2022

View reviewed changes

LinHu2016 force-pushed the supportConcurrentScavenger branch 2 times, most recently from 45f4c13 to 74a8ae0 Compare October 14, 2022 21:06

amicic reviewed Oct 14, 2022

View reviewed changes

LinHu2016 force-pushed the supportConcurrentScavenger branch from 74a8ae0 to 5bb1b21 Compare October 14, 2022 21:19

amicic reviewed Oct 14, 2022

View reviewed changes

LinHu2016 force-pushed the supportConcurrentScavenger branch 2 times, most recently from cb0add1 to a210198 Compare October 17, 2022 02:47

amicic reviewed Oct 17, 2022

View reviewed changes

LinHu2016 force-pushed the supportConcurrentScavenger branch from a210198 to 2d3bdaa Compare October 17, 2022 14:55

dmitripivkine reviewed Oct 17, 2022

View reviewed changes

LinHu2016 force-pushed the supportConcurrentScavenger branch from 2d3bdaa to 2202221 Compare October 17, 2022 17:49

LinHu2016 force-pushed the supportConcurrentScavenger branch from 2202221 to 51d05d6 Compare October 17, 2022 18:10

amicic approved these changes Oct 17, 2022

View reviewed changes

dmitripivkine approved these changes Oct 17, 2022

View reviewed changes

dmitripivkine merged commit 914a3ed into eclipse-openj9:master Oct 19, 2022

amicic mentioned this pull request Oct 20, 2022

Revert "Handle Continuation scan during Concurrent Scavenger and Metronome" #16145

Merged

Handle Continuation scan during Concurrent Scavenger and Metronome #16106

Handle Continuation scan during Concurrent Scavenger and Metronome #16106

Conversation

LinHu2016 commented Oct 14, 2022

LinHu2016 commented Oct 14, 2022

Choose a reason for hiding this comment

amicic commented Oct 14, 2022

amicic commented Oct 14, 2022

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 Oct 14, 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

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

LinHu2016 commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine commented Oct 17, 2022

amicic commented Oct 17, 2022

amicic commented Oct 17, 2022

dmitripivkine commented Oct 18, 2022

amicic commented Oct 18, 2022

dmitripivkine commented Oct 18, 2022

dmitripivkine commented Oct 18, 2022

dmitripivkine commented Oct 18, 2022

dmitripivkine commented Oct 18, 2022

dmitripivkine commented Oct 18, 2022

dmitripivkine commented Oct 19, 2022

dmitripivkine commented Oct 19, 2022

amicic Oct 14, 2022 •

edited

Loading