Loom: GC Thread scanning #15612

LinHu2016 · 2022-07-25T18:53:43Z

Threads should be scanned as they normally would. If a thread is mounted
with a VirtualThread, then the virtual thread stack will be scanned, the
carrier thread stack will be stored in a continuation instance.

currentContinuation field of the J9vmThread point to either NULL or the
continuation with carrier thread stack(mounting with a VirtualThread
case). scanning currentContinuation if needed.

#depends: #15603
#fix: #15179
Signed-off-by: Lin Hu linhu@ca.ibm.com

amicic · 2022-09-16T18:54:07Z

runtime/gc_base/RootScanner.cpp

@@ -536,7 +537,23 @@ MM_RootScanner::scanOneThread(MM_EnvironmentBase *env, J9VMThread* walkThread, v
 doVMThreadSlot(slot, &vmThreadIterator);
 }

- GC_VMThreadStackSlotIterator::scanSlots((J9VMThread *)env->getOmrVMThread()->_language_vmthread, walkThread, localData, stackSlotIterator, isStackFrameClassWalkNeeded(), _trackVisibleStackFrameDepth);
+ J9VMThread *currentThread = (J9VMThread *)env->getOmrVMThread()->_language_vmthread;
+ GC_VMThreadStackSlotIterator::scanSlots(currentThread, walkThread, localData, stackSlotIterator, isStackFrameClassWalkNeeded(), _trackVisibleStackFrameDepth);


You should probably put a comment above, something like this:
In a case this thread is a carrier thread, and a virtual thread is mounted, we will scan virtual thread's stack.

You can append this comment by another sentence.
... If virtual thread is not mounted, or this is just a regular thread, this will scan its own stack.

amicic · 2022-09-16T19:11:54Z

runtime/gc_base/RootScanner.cpp

+ MM_ForwardedHeader forwardHeader(carrierThreadObject, compressed);
+ if (forwardHeader.isForwardedPointer()) {
+ carrierThreadObject = forwardHeader.getForwardedObject();
+ }


You should put a comment, something like this:
At this point we know that a virtual thread is mounted. We previously scanned its stack, and now we need to scan carrier's stack, too. Currently, Continuation struct is pointing to it. doSlot will now only push Continuation object for scanning, what will eventually get Continuation object and struct scanned, but carrier's stack too.

amicic · 2022-09-16T19:19:33Z

runtime/gc_base/RootScanner.cpp

+ bool const compressed = _extensions->compressObjectReferences();
+ j9object_t carrierThreadObject = walkThread->carrierThreadObject;
+ MM_ForwardedHeader forwardHeader(carrierThreadObject, compressed);
+ if (forwardHeader.isForwardedPointer()) {


Typically, carrierThreadObject should have been fixed by now by preceding step that fixes up thread struct slots. As we discussed, the only reason we think this is possible is because of recently remembered optimization, that may defer the fixup. It will occur if carrierThreadObject just got tenured. Let's confirme that's the case.

Either way, this is not an acceptable fix (we cannot have scavenger specific code in a middle of common root scanner). We could do override this method (or part of it) in ScavengerRootScanner and do that extra check only there.

amicic · 2022-09-26T17:21:43Z

runtime/gc_structs/VMThreadStackSlotIterator.cpp

+
+ if (includeStackFrameClassReferences) {
+ stackWalkState.flags |= J9_STACKWALK_ITERATE_METHOD_CLASS_SLOTS;
+ }


could you introduce a local method like initializeStackWalkState(ForGC?) and share this pattern with 2 other scanSlots methods in this file?

amicic · 2022-09-26T17:34:41Z

runtime/gc_base/RootScanner.cpp

+ {
+ /* At this point we know that a virtual thread is mounted. We previously scanned its stack, and now we need to scan carrier's stack, too.
+ * Currently, Continuation struct is pointing to it. doSlot will now only push Continuation object for scanning,
+ * what will eventually get Continuation object and struct scanned, but carrier's stack too. */


You can now simplify this comment, something like:

At this point we know that a virtual thread is mounted. We previously scanned its stack, and now we will scan carrier's stack, that continuation struct is currently pointing to.

amicic · 2022-09-26T17:38:09Z

runtime/gc_glue_java/ScavengerThreadRescanner.hpp

@@ -60,7 +60,9 @@ class MM_ScavengerThreadRescanner : public MM_RootScanner

 virtual void doSlot(omrobjectptr_t *slotPtr) {
 /* we only process thread and stack slots with this scanner */
- Assert_MM_unreachable();
+ /* but for mounted vthread case, process related continuation Object */
+ _scavenger->rescanThreadSlot(MM_EnvironmentStandard::getEnvironment(_env), slotPtr);


do we still need this?

I have rolled back the change.

amicic · 2022-09-27T13:54:53Z

runtime/gc_structs/VMThreadStackSlotIterator.cpp

@@ -63,6 +63,41 @@ vmThreadStackFrameIterator(J9VMThread * currentThread, J9StackWalkState * walkSt

 } /* extern "C" */

+void


Either make this is a static method or make the name of the method more specific for GC purposes. In theory there could be a same/similar method somewhere in core VM code.

as I said in hpp comment, make this method a class member and remove 'forGC'

amicic · 2022-09-27T14:01:39Z

runtime/gc_structs/VMThreadStackSlotIterator.cpp

- stackWalkState.userData3 = userData;
-
- stackWalkState.flags = J9_STACKWALK_ITERATE_O_SLOTS | J9_STACKWALK_DO_NOT_SNIFF_AND_WHACK;
+ initializeStackWalkState(&stackWalkState, vmThread, userData, oSlotIterator, includeStackFrameClassReferences, trackVisibleFrameDepth);
 stackWalkState.walkThread = walkThread;


should we set this explicitely to NULL in other cases?

amicic · 2022-09-27T14:13:01Z

runtime/gc_structs/VMThreadStackSlotIterator.hpp

@@ -43,6 +43,14 @@ typedef void J9MODRON_OSLOTITERATOR(J9JavaVM *javaVM, J9Object **objectIndirect,
 */
 class GC_VMThreadStackSlotIterator
 {
+private:
+ static void initializeStackWalkStateForGC(


If you make it a class member, then you don't need 'forGC'. The class name gives it specific enough scope.

Not sure how this compiles though, since at the spot the method is defined in cpp file, it does not seem like the method is a member of the class (what made me say it needs more specific scope)!?

ah, I understand now.... in *cpp this method is defined before their users, so declaration was not needed.
this declaration is unused
but it's probably the best approach: leave this declaration, remove 'forGC' (since the class gives specific scope) and fix cpp file so that method is part of this class

amicic · 2022-09-27T16:30:26Z

runtime/gc_structs/VMThreadStackSlotIterator.cpp

- stackWalkState.userData3 = userData;
-
- stackWalkState.flags = J9_STACKWALK_ITERATE_O_SLOTS | J9_STACKWALK_DO_NOT_SNIFF_AND_WHACK;
+ GC_VMThreadStackSlotIterator::initializeStackWalkState(&stackWalkState, vmThread, userData, oSlotIterator, includeStackFrameClassReferences, trackVisibleFrameDepth);


Sorry for being picky and not clear enough, but we don't need the class scope operator here (and 2 other spots). My original concern was only about the symbol definition (and potential overlap with a same symbol somewhere else).

amicic · 2022-09-27T16:33:44Z

Please, update commit/PR comment. For example, I believe this is not needed any more:

scanning Continuation Object in Heap( mounted j9vmThread->threadObject
(virtualThread Object)--> the continuation Object), scanning
currentContinuation is unnecessary for "normal case", but it would help
generational GC case(we don't need to add tenured continuation Object in
rememberedSet if it mount to the carrier thread).

amicic · 2022-09-27T17:18:31Z

Jenkins test sanity xLinux jdk19

amicic · 2022-09-27T17:18:51Z

jenkins compile win jdk19

Threads should be scanned as they normally would. If a thread is mounted with a VirtualThread, then the virtual thread stack will be scanned, the carrier thread stack will be stored in a continuation instance. currentContinuation field of the J9vmThread point to either NULL or the continuation with carrier thread stack(mounting with a VirtualThread case). scanning currentContinuation if needed. Signed-off-by: Lin Hu <linhu@ca.ibm.com>

LinHu2016 · 2022-09-27T18:38:32Z

Jenkins test sanity xLinux jdk19

dmitripivkine · 2022-09-27T18:54:03Z

Jenkins test sanity xLinux jdk19

dmitripivkine · 2022-09-27T18:54:19Z

jenkins compile win jdk19

LinHu2016 changed the title ~~WIP:Loom: GC Thread scanning~~ Loom: GC Thread scanning Aug 23, 2022

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch 7 times, most recently from b08e9c1 to fa55b2a Compare September 11, 2022 23:53

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

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch 3 times, most recently from bcc628e to d5bd0d4 Compare September 16, 2022 17:31

amicic reviewed Sep 16, 2022

View reviewed changes

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch 3 times, most recently from a609c4f to 398f84d Compare September 26, 2022 15:36

amicic reviewed Sep 26, 2022

View reviewed changes

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch 2 times, most recently from 2b1199d to 44b7049 Compare September 27, 2022 13:49

amicic reviewed Sep 27, 2022

View reviewed changes

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch from 44b7049 to 23a7b3b Compare September 27, 2022 13:56

amicic reviewed Sep 27, 2022

View reviewed changes

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch from 23a7b3b to bf33015 Compare September 27, 2022 14:08

amicic reviewed Sep 27, 2022

View reviewed changes

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch from bf33015 to 16a38d5 Compare September 27, 2022 15:19

amicic reviewed Sep 27, 2022

View reviewed changes

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch 2 times, most recently from 372f4d8 to 660f727 Compare September 27, 2022 17:15

amicic approved these changes Sep 27, 2022

View reviewed changes

LinHu2016 force-pushed the Loom_GC_ThreadScanning branch from 660f727 to 26a25f9 Compare September 27, 2022 17:59

amicic merged commit 53931da into eclipse-openj9:master Sep 28, 2022

pshipton mentioned this pull request Sep 28, 2022

jdk19 Jep425Tests_testVirtualThread_0 hangs #16011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loom: GC Thread scanning #15612

Loom: GC Thread scanning #15612

LinHu2016 commented Jul 25, 2022 •

edited

Loading

amicic Sep 16, 2022 •

edited

Loading

amicic Sep 26, 2022

amicic Sep 16, 2022 •

edited

Loading

amicic Sep 16, 2022 •

edited

Loading

amicic Sep 26, 2022 •

edited

Loading

amicic Sep 26, 2022 •

edited

Loading

amicic Sep 26, 2022

LinHu2016 Sep 27, 2022

amicic Sep 27, 2022

amicic Sep 27, 2022

amicic Sep 27, 2022

amicic Sep 27, 2022

amicic Sep 27, 2022 •

edited

Loading

amicic Sep 27, 2022

amicic Sep 27, 2022 •

edited

Loading

amicic commented Sep 27, 2022

amicic commented Sep 27, 2022

amicic commented Sep 27, 2022

LinHu2016 commented Sep 27, 2022

dmitripivkine commented Sep 27, 2022

dmitripivkine commented Sep 27, 2022

		@@ -63,6 +63,41 @@ vmThreadStackFrameIterator(J9VMThread * currentThread, J9StackWalkState * walkSt

		} /* extern "C" */

		void

Loom: GC Thread scanning #15612

Loom: GC Thread scanning #15612

Conversation

LinHu2016 commented Jul 25, 2022 • edited Loading

amicic Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

amicic Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

amicic Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

amicic Sep 26, 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 Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

amicic commented Sep 27, 2022

amicic commented Sep 27, 2022

amicic commented Sep 27, 2022

LinHu2016 commented Sep 27, 2022

dmitripivkine commented Sep 27, 2022

dmitripivkine commented Sep 27, 2022

LinHu2016 commented Jul 25, 2022 •

edited

Loading

amicic Sep 16, 2022 •

edited

Loading

amicic Sep 16, 2022 •

edited

Loading

amicic Sep 16, 2022 •

edited

Loading

amicic Sep 26, 2022 •

edited

Loading

amicic Sep 26, 2022 •

edited

Loading

amicic Sep 27, 2022 •

edited

Loading

amicic Sep 27, 2022 •

edited

Loading