Pruning the cards from InterRegionRememberedsets after GMP #8804

LinHu2016 · 2020-03-09T16:17:11Z

After Global Marking Phase, the cards reference from "free memory"
should be pruned from InterRegionRememberedsets, in order to
reuse the free memory in future, currently we have card refresh
and card pruning in beginning PGC, so piggyback existing logic
add extra condition for pruning cards related with "free memory"
in first PGC after GMP.

- During Rememberedsets flush in the beginning PGC(for
collection set), extra condition check for dirty card (do not
dirty card if corresponding MarkMap is 0)
- During global Rememberedsets prune, extra condition check
for pruning the card (do remove the card if corresponding
MarkMap is 0).

depends on: eclipse-omr/omr#4928
fix partial #9247

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

amicic · 2020-03-11T17:28:24Z

runtime/gc_vlhgc/CardListFlushTask.cpp

+							bool needToFlush = true;
+							if (needToCheckMarkMap) {
+								void* heapAddress = convertHeapAddressFromRememberedSetCard(envBase, card);
+								uint64_t* map4Card = (uint64_t*) &((uintptr_t *)(markMap->getMarkBits()))[markMap->getSlotIndex((omrobjectptr_t) heapAddress)];


this needs to go to MarkMap class, a helper like:
bool anyLiveObjectInCard(void *heapAddress);

and state in a comment that implementation assumes that card covers exactly 512 bytes, 1 mark bit is 8 bytes of heap and heap address is card size aligned.

you can add compile time assert that BITS_PER_BYTE is 8 and CARD_SIZE_SHIFT is 9

amicic · 2020-03-11T17:37:54Z

runtime/gc_vlhgc/CardListFlushTask.cpp

-							writeFlushToCardState(cardAddress, gmpIsActive);
+							bool needToFlush = true;
+							if (needToCheckMarkMap) {
+								void* heapAddress = convertHeapAddressFromRememberedSetCard(envBase, card);


interRegionRememberedSet is available. you can use its convertHeapAddressFromRememberedSetCard(), and no need to clone it in this class

amicic · 2020-03-11T19:59:26Z

runtime/gc_vlhgc/InterRegionRememberedSet.cpp

+						needToRemove = true;
+					}
+
+					if (!needToRemove && needToCheckMarkMap) {


The new check is just more generic variant of fromRegion->containsObjects().
So I recommend you introduce a helper that will replace that containsObjects() check:

inline bool IRRS::cardContainsObjects(card, fromRegion, needToCheckMarkMap) {
if needToCheckMarkMap
return markMap->anyLiveObjectInCard(card)
else
return !fromRegion->containsObjects()
}

We can later consider checking against MarkMap all the time (not just after GMP), if it's not much slower. But that might be tricky since for evacuated regions clearing of mark map might be done late(r).

The new check is just more generic variant of fromRegion->containsObjects().
So I recommend you introduce a helper that will replace that containsObjects() check:

inline bool IRRS::cardContainsObjects(card, fromRegion, needToCheckMarkMap) {
if needToCheckMarkMap
return markMap->anyLiveObjectInCard(card)
else
return !fromRegion->containsObjects()
}

good idea! it is good for clearFromRegionReferencesForMarkDirect(), but for clearFromRegionReferencesForMarkOptimized() (it is default, using compressed card table ), it is little bit tricky, partial logic for checking empty region is in rebuildCompressedCardTableForMark(), we can add CheckMarkMap in rebuildCompressedCardTableForMark, but it need to check every card in the regions, it might be expensive?

I see. Actually, I think it is CompressedCardTable::rebuildCompressedCardTableForPartialCollect that would be more impacted. We could try that later and see if it will slow it down.

Still I think you can introduce the helper and use it here, and in 'else' clause of 'if (compressedCardTable->isReady()) {', while the existing areAnyLiveObjectsInCard() check you can move up in 'then' clause of 'if (compressedCardTable->isReady()) {'

if we use the helper in "else" clause, so we have to add the logic in rebuildCompressedCardTableForPartialCollect(), otherwise if compressedCardTable->isReady() case, we would miss to check markMap.

if (compressedCardTable->isReady()) { .... } else { }

amicic · 2020-03-11T21:46:20Z

We need to think/test if Mark Map based check is needed in first PGC after a Mark-Compact based PGC, since it may also create populated regions with empty chunks (at least a tail, if not a chunk in a middle).

LinHu2016 · 2020-03-12T16:09:44Z

We need to think/test if Mark Map based check is needed in first PGC after a Mark-Compact based PGC, since it may also create populated regions with empty chunks (at least a tail, if not a chunk in a middle).

there is pruning card at the beginning of compact, clearFromRegionReferencesForCompact(), not sure if it covered the cases.

amicic · 2020-03-27T19:23:20Z

runtime/gc_vlhgc/CardListFlushTask.cpp

-							Card *cardAddress = interRegionRememberedSet->rememberedSetCardToCardAddr(env, card);
-							writeFlushToCardState(cardAddress, gmpIsActive);
+						if (interRegionRememberedSet->cardMayContainObjects(card, referencingRegion, needToCheckMarkMap, markMap) && !referencingRegion->_markData._shouldMark) {
+							bool needToFlush = true;


no need for needToFlush flag, any more. here, and at a couple of other similar spots.

amicic · 2020-03-27T19:23:50Z

runtime/gc_vlhgc/InterRegionRememberedSet.cpp

-					if (fromRegion->_markData._shouldMark  || !fromRegion->containsObjects() || isDirtyCardForPartialCollect(env, cardTable, cardAddress)) {
+					/* Regions that are completely swept after a GMP, might still have outgoing references (thus we consider cardContainsNoObjects) */
+					if (fromRegion->_markData._shouldMark  || !cardMayContainObjects(card, fromRegion, needToCheckMarkMap, markMap) || isDirtyCardForPartialCollect(env, cardTable, cardAddress)) {
+						needToRemove = true;


no need for needToRemove

amicic · 2020-03-27T19:26:20Z

runtime/gc_vlhgc/InterRegionRememberedSet.hpp

+	 * Helper function isCompressedCardDirtyForPartialCollect
+	 * extended from compressedCardTable->isCompressedCardDirtyForPartialCollect(), only in case first PGC after GMP, also dirty card if card Contains No Object.
+	 */
+	MMINLINE bool isCompressedCardDirtyForPartialCollect(MM_EnvironmentVLHGC* env, UDATA card, bool needToCheckMarkMap)


pass compressedCardTable and mark map

amicic · 2020-03-27T19:35:03Z

runtime/gc_vlhgc/InterRegionRememberedSet.cpp

@@ -933,11 +935,12 @@ MM_InterRegionRememberedSet::clearFromRegionReferencesForMarkDirect(MM_Environme
 				while(0 != (card = rsclCardIterator.nextReferencingCard(env))) {
 					MM_HeapRegionDescriptorVLHGC *fromRegion = tableDescriptorForRememberedSetCard(card);
 					Card * cardAddress = rememberedSetCardToCardAddr(env, card);
-					/* Regions that are completely swept after a GMP, might still have outgoing references (thus we consider empty regions too) */
-					if (fromRegion->_markData._shouldMark  || !fromRegion->containsObjects() || isDirtyCardForPartialCollect(env, cardTable, cardAddress)) {
+					/* Regions that are completely swept after a GMP, might still have outgoing references (thus we consider cardContainsNoObjects) */


This whole comment should be updated:
Regions that are completely swept or parts of regions that are partially swept after a GMP might still have outgoing references. If they are (if they may not contain objects), cards should be removed.

amicic · 2020-03-27T20:40:56Z

runtime/gc_vlhgc/InterRegionRememberedSet.hpp

+	 * if this is first PGC after GMP, check if markMap->areAnyLiveObjectsInCard()
+	 * otherwise check if region->containsObjects()
+	 */
+	MMINLINE bool cardMayContainObjects(UDATA card, MM_HeapRegionDescriptorVLHGC *fromRegion, bool needToCheckMarkMap, MM_MarkMap *markMap)


both helpers could be static (hence saving by not passing implicit 'this' pointer)

amicic · 2020-03-27T20:49:53Z

runtime/gc_vlhgc/CardListFlushTask.cpp

@@ -96,6 +97,11 @@ MM_CardListFlushTask::run(MM_EnvironmentBase *envBase)
 {
 	MM_EnvironmentVLHGC *env = MM_EnvironmentVLHGC::getEnvironment(envBase);
 	MM_GCExtensions *extensions = MM_GCExtensions::getExtensions(env);
+	MM_MarkMap *markMap = NULL;
+	if (static_cast<MM_CycleStateVLHGC*>(envBase->_cycleState)->_schedulingDelegate->isFirstPGCAfterGMP())
+	{


move open bracket to previous line

your fixed the other two, but not this one :)

amicic · 2020-03-27T20:50:21Z

runtime/gc_vlhgc/InterRegionRememberedSet.cpp

+	MM_CardTable *cardTable = extensions->cardTable;
+	MM_MarkMap *markMap = NULL;
+	if (static_cast<MM_CycleStateVLHGC*>(env->_cycleState)->_schedulingDelegate->isFirstPGCAfterGMP())
+	{


move bracket

amicic · 2020-03-27T20:50:38Z

runtime/gc_vlhgc/InterRegionRememberedSet.cpp

+	MM_CompressedCardTable *compressedCardTable = extensions->compressedCardTable;
+	MM_MarkMap *markMap = NULL;
+	if (static_cast<MM_CycleStateVLHGC*>(env->_cycleState)->_schedulingDelegate->isFirstPGCAfterGMP())
+	{


amicic · 2020-03-27T20:57:59Z

runtime/gc_vlhgc/InterRegionRememberedSet.hpp

+
+	/**
+	 * Helper function isCompressedCardDirtyForPartialCollect
+	 * extended from compressedCardTable->isCompressedCardDirtyForPartialCollect(), only in case first PGC after GMP, also dirty card if card Contains No Object.


comment says 'also dirty card'
did you mean 'also remove card'?

compressedCardTable->isCompressedCardDirtyForPartialCollect() return true if the card is dirty, extra condition if the card Contains No Object return true (the card is dirty)

Ah, then it should say 'also return as dirty'. As-is I understood you imply an action of dirtying the card.

amicic · 2020-03-27T21:39:14Z

runtime/gc_vlhgc/InterRegionRememberedSet.hpp

+	{
+		void* heapAddress = convertHeapAddressFromRememberedSetCard(card);
+		bool ret = compressedCardTable->isCompressedCardDirtyForPartialCollect(env, heapAddress);
+		if (!ret && (NULL != markMap)) {


add comment:
TODO: consider incorporating areAnyLiveObjectsInCard info when building Compressed Card Table

amicic · 2020-03-27T21:55:16Z

Jenkins test sanity all jdk8

amicic · 2020-03-27T22:21:38Z

runtime/gc_vlhgc/InterRegionRememberedSet.hpp

+	 * if this is first PGC after GMP, check if markMap->areAnyLiveObjectsInCard()
+	 * otherwise check if region->containsObjects()
+	 */
+	MMINLINE static bool cardMayContainObjects(UDATA card, MM_HeapRegionDescriptorVLHGC *fromRegion, MM_MarkMap *markMap)


These helpers won't be able to be static, afterall :( . It may work now, but once OMR_GC_FULL_POINTERS is enabled, they will indirectly rely on class member _compressObjectReferences

After Global Marking Phase, the cards reference from "free memory" should be pruned from InterRegionRememberedsets, in order to reuse the free memory in future, currently we have card refresh and card pruning in beginning PGC, so piggyback existing logic add extra condition for pruning cards related with "free memory" in first PGC after GMP. - During Rememberedsets flush in the beginning PGC(for collection set), extra condition check for dirty card (do not dirty card if corresponding MarkMap is 0) - During global Rememberedsets prune, extra condition check for pruning the card (do remove the card if corresponding MarkMap is 0). Signed-off-by: Lin Hu <linhu@ca.ibm.com>

amicic · 2020-03-27T23:08:55Z

Jenkins test sanity all jdk8

dmitripivkine added the comp:gc label Mar 9, 2020

dmitripivkine requested a review from amicic March 9, 2020 16:51

dmitripivkine assigned amicic Mar 9, 2020

amicic reviewed Mar 11, 2020

View reviewed changes

LinHu2016 force-pushed the GMP_clearingRS branch from 7b3c5eb to 9469d5a Compare March 12, 2020 15:16

LinHu2016 force-pushed the GMP_clearingRS branch 4 times, most recently from 32d98bc to 84d4894 Compare March 18, 2020 12:21

LinHu2016 force-pushed the GMP_clearingRS branch from 84d4894 to d1c00ab Compare March 26, 2020 12:22

LinHu2016 mentioned this pull request Mar 26, 2020

WIP: Reuse free memory from GMP in PGC #7032

Closed

LinHu2016 force-pushed the GMP_clearingRS branch 6 times, most recently from 499e655 to 47aa9c2 Compare March 27, 2020 19:14