GC update for supporting yield of pinned VirtualThread(JEP491) #21147
Conversation
LinHu2016
force-pushed
the
update4virtualThreadMonitor
branch
3 times, most recently
from
4717fc3 to
0bd5700
Compare
LinHu2016
force-pushed
the
update4virtualThreadMonitor
branch
from
0bd5700 to
f6f7a59
Compare
| @@ -45,10 +45,12 @@ MM_MarkingSchemeRootMarker::doStackSlot(omrobjectptr_t *slotPtr, void *walkState | |||
| omrobjectptr_t object = *slotPtr; | |||
| if (_markingScheme->isHeapObject(object) && !_extensions->heap->objectIsInGap(object)) { | |||
| /* heap object - validate and mark */ | |||
| Assert_MM_validStackSlot(MM_StackSlotValidator(0, object, stackLocation, walkState).validate(_env)); | |||
| if (NULL != walkState) { | |||
There was a problem hiding this comment.
don't think we should try to share (plus even the name with 'stack' in it is not appropriate, when calling from GC_ContinuationSlotIterator)
we don't need either 'is heap object' or 'does walkState exist' complexity for Continuation object slots
simply just use plan doSlot. in this case it already exists, but create one for other delegates if don't exist
There was a problem hiding this comment.
to be even cleaner you we should have both doContinuationSlot and doSlot, where the former would just call the latter. the latter could be used for different purposes, while the former is specific (and potentially can do a bit more then just call doSlot)
There was a problem hiding this comment.
for consistency doContinuationSlot for any of delegates should pass Continuation object (or native struct?), even though most of the time it would not be needed
| @@ -72,5 +75,15 @@ MM_HeapWalkerDelegate::doContinuationNativeSlots(MM_EnvironmentBase *env, omrobj | |||
| localData.userData = userData; | |||
|
|
|||
| GC_VMThreadStackSlotIterator::scanContinuationSlots(currentThread, objectPtr, (void *)&localData, stackSlotIteratorForHeapWalker, false, false); | |||
|
|
|||
| #if JAVA_SPEC_VERSION >= 24 | |||
| J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(currentThread, objectPtr); | |||
There was a problem hiding this comment.
we fetch native struct even within GC_VMThreadStackSlotIterator::scanContinuationSlots
consider just doing it once and pass the native rather then object to it
seems like object is not needed in GC_VMThreadStackSlotIterator::scanContinuationSlots
low priority - could be even done later
| ret = ¤tMonitorRecord->object; | ||
| } else if (NULL != _vthread) { | ||
| ret = _vthread; | ||
| _vthread = NULL; |
There was a problem hiding this comment.
i'll defer this to @dmitripivkine if this 'chained if' is good enough (only 3 cases) or we want some kind of state or index to drive through 3 cases more directly (like in GC_VMThreadIterator::nextSlot() )
There was a problem hiding this comment.
I think this implementation is clear enough. We can create iterator states if you like.
There was a problem hiding this comment.
ok, let's keep it
but some formatting should be cleaned up:
- NULL should be left of binary operators
- there is extra empty spaces at line 50
| Assert_MM_validStackSlot(MM_StackSlotValidator(MM_StackSlotValidator::COULD_BE_FORWARDED, *slotPtr, stackLocation, walkState).validate(env)); | ||
| thread = ((J9StackWalkState *)walkState)->currentThread; | ||
| } else { | ||
| thread = (J9VMThread *) stackLocation; |
There was a problem hiding this comment.
this hack won't be needed once you create specific doContinuationSlot that will pass Continuation object on which reserving context should be based (note also that there is specific doVMThreadSlot that passes GC_VMThreadIterator for the same purpose - this case is more analogous to ours, than doStackSlot case is)
we can't even talk about thread reserving context, since continuations are not bound to a particular carrier thread
we actually have a missing feature here:
- continuation objects should have context (and they do, being in a region which they have context)
- carrier thread should have context and they should be equally distributed among them (and they probably are as any other mutator thread)
- during mount we should prefer to mount continuation on matching carrier context (if multiple of them are free to run) - we don't have this optimization, but we don't want to address it now
LinHu2016
force-pushed
the
update4virtualThreadMonitor
branch
from
f6f7a59 to
9d00fcd
Compare
In order to supporting yield of pinned VirtualThread a couple of heap references have been added in chain of continuation structures, GC need to maintain(scan/update) these references. 1, new ContunuationSlotIterator to iterate all of the references list of monitorRecord list of jniMonitorRecord vthread reference 2, (temporary hack) piggyback doStackSlot() to handle slot processing, pass NULL for walkState to avoid stack slots specific process, pass vmThread* as stackLocation for copyforward case to retrieve the reservingContext bound to nodes. 3, mounted continuations are scanned during rootscan/scanOneThread unmounted continuations are scanned during ContinuationNativeSlots Signed-off-by: lhu <[email protected]>
Signed-off-by: lhu <[email protected]>
LinHu2016
force-pushed
the
update4virtualThreadMonitor
branch
from
9d00fcd to
69ae24f
Compare
Signed-off-by: lhu <[email protected]>
In order to supporting yield of pinned VirtualThread
a couple of heap references have been added in chain of continuation
structures, GC need to maintain(scan/update) these references.
1, new ContunuationSlotIterator to iterate all of the references
list of monitorRecord
list of jniMonitorRecord
vthread reference
2, (temporary hack) piggyback doStackSlot() to handle slot processing,
pass NULL for walkState to avoid stack slots specific process,
pass vmThread* as stackLocation for copyforward case to retrieve the
reservingContext bound to nodes.
3, mounted continuations are scanned during rootscan/scanOneThread
unmounted continuations are scanned during scanContinuationNativeSlots
Signed-off-by: lhu [email protected]