Skip to content

Commit d3082c3

Browse files
committed
Fix PGC edenSurvivalRate overflow issues in CopyForward
The edenSurvivalRate represents the fraction of Eden regions that survived a PGC (nominally in the range [0.0, 1.0]). An overflow (rate > 1.0) can cause incorrect heap/Eden size adjustments, producing unexpected allocation spikes and, in extreme cases, out-of-memory errors. The issue is not a regression but was exposed by a recent Eden-resize change. Four overflow sources are addressed: 1. Incorrect edenCountBeforeCollect (denominator) During a PGC the heap may expand, triggering an Eden size recalculation. The newly calculated Eden size can differ significantly from the size at collection start, making the survival rate inaccurate. Fix: use copyForwardStats->_edenEvacuateRegionCount as the denominator instead of getCurrentEdenSizeInRegions(env). 2. Semantic mismatch between edenSurvivorCount and edenEvacuateCount In the first few PGCs after a GMP, previous Eden survivors are ADDRESS_ORDERED_MARKED regions with logicalAge == 0. They enter the evacuate set as non-Eden regions, but their surviving objects spill into age-0 survivor regions that are counted as Eden survivors. This can cause _edenSurvivorRegionCount > _edenEvacuateRegionCount. Fix: add _hasEdenContent to MM_CopyScanCacheVLHGC and MM_HeapRegionDescriptorVLHGC::_copyForwardData. The flag is set during copy() when the source region isEden() (ADDRESS_ORDERED), propagated from cache to region in stopCopyingIntoCache(), and used in postProcessRegions() to classify survivor regions accurately. 3. Over-counting of Eden survivor regions (shared fresh survivors) Because Eden and age-0 non-Eden regions share the same compact group, they can fill the same fresh survivor regions, causing _edenSurvivorRegionCount to be over-counted. Fix: redistribute edenSurvivorCount and nonEdenSurvivorCount using _copyBytesEden as a ceiling-division estimate of the true Eden survivor region count. 4. _scanBytesEden double-counting (cap) During abort recovery, objects that were partially copied (forwarding pointer already installed) are scanned again in the source region, double-counting their bytes in _scanBytesEden. Combined with high survival rates, alignment waste, or NUMA/hybrid no-evacuation cases, this can push edenSurvivorCount above edenCountBeforeCollect. Fix: cap edenSurvivorCount at edenCountBeforeCollect and add an assertion that thisSurvivalRate <= 1.0. Signed-off-by: lhu <linhu@ca.ibm.com>
1 parent 5d0c227 commit d3082c3

5 files changed

Lines changed: 59 additions & 3 deletions

File tree

runtime/gc_vlhgc/CopyForwardScheme.cpp

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -493,6 +493,8 @@ MM_CopyForwardScheme::preProcessRegions(MM_EnvironmentVLHGC *env)
493493
while (NULL != (region = regionIterator.nextRegion())) {
494494
region->_copyForwardData._survivor = false;
495495
region->_copyForwardData._freshSurvivor = false;
496+
region->_copyForwardData._hasEdenContent = false;
497+
496498
_regionShouldMark[_regionManager->mapDescriptorToRegionTableIndex(region)] = region->_markData._shouldMark;
497499
if (region->containsObjects()) {
498500
region->_copyForwardData._initialLiveSet = true;
@@ -541,8 +543,13 @@ MM_CopyForwardScheme::postProcessRegions(MM_EnvironmentVLHGC *env)
541543
static_cast<MM_CycleStateVLHGC *>(env->_cycleState)->_vlhgcIncrementStats._copyForwardStats._nonEdenEvacuateRegionCount += 1;
542544
}
543545
} else if (region->isFreshSurvivorRegion()) {
544-
/* check Eden Survivor Regions */
545-
if (0 == region->getLogicalAge()) {
546+
/* Classify the survivor region by the Eden-ness of its source objects.
547+
* _hasEdenContent is set during copy when sourceRegion->isEden() (ADDRESS_ORDERED type),
548+
* which is the same predicate used on the evacuate side. This avoids the previous bug
549+
* where logicalAge==0 was used instead, which incorrectly included survivors from
550+
* non-Eden regions at compact group 0 (type ADDRESS_ORDERED_MARKED).
551+
*/
552+
if (region->_copyForwardData._hasEdenContent) {
546553
static_cast<MM_CycleStateVLHGC *>(env->_cycleState)->_vlhgcIncrementStats._copyForwardStats._edenSurvivorRegionCount += 1;
547554
} else {
548555
static_cast<MM_CycleStateVLHGC *>(env->_cycleState)->_vlhgcIncrementStats._copyForwardStats._nonEdenSurvivorRegionCount += 1;
@@ -582,6 +589,7 @@ MM_CopyForwardScheme::postProcessRegions(MM_EnvironmentVLHGC *env)
582589
region->_copyForwardData._requiresPhantomReferenceProcessing = false;
583590
region->_copyForwardData._survivor = false;
584591
region->_copyForwardData._freshSurvivor = false;
592+
region->_copyForwardData._hasEdenContent = false;
585593

586594
if (region->_copyForwardData._evacuateSet) {
587595
Assert_MM_true(region->_sweepData._alreadySwept);
@@ -708,6 +716,7 @@ MM_CopyForwardScheme::reinitCache(MM_EnvironmentVLHGC *env, MM_CopyScanCacheVLHG
708716

709717
Assert_MM_true(compactGroup < _compactGroupMaxCount);
710718
cache->_compactGroup = compactGroup;
719+
cache->_hasEdenContent = false;
711720
Assert_MM_true(0.0 == cache->_allocationAgeSizeProduct);
712721

713722
MM_HeapRegionDescriptorVLHGC * region = (MM_HeapRegionDescriptorVLHGC *)_regionManager->tableDescriptorForAddress(cache->cacheBase);
@@ -1859,6 +1868,12 @@ MM_CopyForwardScheme::stopCopyingIntoCache(MM_EnvironmentVLHGC *env, uintptr_t c
18591868

18601869
MM_HeapRegionDescriptorVLHGC * region = (MM_HeapRegionDescriptorVLHGC *)_regionManager->tableDescriptorForAddress(copyCache->cacheBase);
18611870

1871+
/* propagate Eden-content flag: a region is an Eden-content survivor if any cache that filled it received Eden objects */
1872+
if (copyCache->_hasEdenContent) {
1873+
region->_copyForwardData._hasEdenContent = true;
1874+
}
1875+
copyCache->_hasEdenContent = false;
1876+
18621877
/* atomically add (age * usedBytes) product from this cache to the regions product */
18631878
double newAllocationAgeSizeProduct = region->atomicIncrementAllocationAgeSizeProduct(copyCache->_allocationAgeSizeProduct);
18641879
region->updateAgeBounds(copyCache->_lowerAgeBound, copyCache->_upperAgeBound);
@@ -2094,6 +2109,7 @@ MM_CopyForwardScheme::copy(MM_EnvironmentVLHGC *env, MM_AllocationContextTarok *
20942109
env->_copyForwardCompactGroups[sourceCompactGroup]._edenStats._liveBytes += objectCopySizeInBytes;
20952110
env->_copyForwardCompactGroups[destinationCompactGroup]._edenStats._copiedObjects += 1;
20962111
env->_copyForwardCompactGroups[destinationCompactGroup]._edenStats._copiedBytes += objectCopySizeInBytes;
2112+
copyCache->_hasEdenContent = true;
20972113
} else {
20982114
env->_copyForwardCompactGroups[sourceCompactGroup]._nonEdenStats._liveObjects += 1;
20992115
env->_copyForwardCompactGroups[sourceCompactGroup]._nonEdenStats._liveBytes += objectCopySizeInBytes;

runtime/gc_vlhgc/CopyScanCacheVLHGC.hpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ class MM_CopyScanCacheVLHGC : public MM_CopyScanCache
5353
uint64_t _lowerAgeBound; /**< lowest possible age of any object in this copy cache */
5454
uint64_t _upperAgeBound; /**< highest possible age of any object in this copy cache */
5555
uintptr_t _arraySplitIndex; /**< The index within the array in scanCurrent to start scanning from (meaningful is OMR_COPYSCAN_CACHE_TYPE_SPLIT_ARRAY is set) */
56+
bool _hasEdenContent; /**< true if at least one object copied into this cache originated from an Eden (ADDRESS_ORDERED) source region */
5657

5758
/* Members Function */
5859
private:
@@ -88,6 +89,7 @@ class MM_CopyScanCacheVLHGC : public MM_CopyScanCache
8889
, _lowerAgeBound(U_64_MAX)
8990
, _upperAgeBound(0)
9091
, _arraySplitIndex(0)
92+
, _hasEdenContent(false)
9193
{}
9294
};
9395

runtime/gc_vlhgc/HeapRegionDescriptorVLHGC.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ MM_HeapRegionDescriptorVLHGC::initialize(MM_EnvironmentBase *env, MM_HeapRegionM
8383
_copyForwardData._survivor = false;
8484
_copyForwardData._freshSurvivor = false;
8585
_copyForwardData._nextRegion = NULL;
86+
_copyForwardData._hasEdenContent = false;
8687
_copyForwardData._previousRegion = NULL;
8788

8889
#if defined (J9VM_GC_MODRON_COMPACTION)

runtime/gc_vlhgc/HeapRegionDescriptorVLHGC.hpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ class MM_HeapRegionDescriptorVLHGC : public MM_HeapRegionDescriptor
7171
bool _freshSurvivor;
7272
MM_HeapRegionDescriptorVLHGC *_nextRegion; /**< Region list link for compact group resource management during a copyforward operation */
7373
MM_HeapRegionDescriptorVLHGC *_previousRegion; /**< Region list link for compact group resource management during a copyforward operation */
74+
bool _hasEdenContent; /**< true if at least one object copied into this survivor region originated from an Eden (ADDRESS_ORDERED) source region */
7475
} _copyForwardData;
7576
#if defined (J9VM_GC_MODRON_COMPACTION)
7677
MM_HeapRegionDataForCompactVLHGC _compactData;

runtime/gc_vlhgc/SchedulingDelegate.cpp

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,15 @@ MM_SchedulingDelegate::partialGarbageCollectCompleted(MM_EnvironmentVLHGC *env,
372372
bool globalSweepHappened = _globalSweepRequired;
373373
_globalSweepRequired = false;
374374
/* copy out the Eden size of the previous interval (between the last PGC and this one) before we recalculate the next one */
375-
uintptr_t edenCountBeforeCollect = getCurrentEdenSizeInRegions(env);
375+
/* During GC, the heap may expand to allow the current collection to continue.
376+
* A heap resize triggers an eden size recalculation without allowing further
377+
* heap expansion (heap reconfiguration). As a result, the newly calculated
378+
* eden size can differ significantly from the eden size at the start of the
379+
* collection, causing the SurvivalRate calculation to become inaccurate.
380+
* Therefore, set copyForwardStats->_edenEvacuateRegionCount (instead of getCurrentEdenSizeInRegions(env)) to
381+
* edenCountBeforeCollect.
382+
*/
383+
uintptr_t edenCountBeforeCollect = copyForwardStats->_edenEvacuateRegionCount;
376384

377385
Trc_MM_SchedulingDelegate_partialGarbageCollectCompleted_stats(env->getLanguageVMThread(),
378386
copyForwardStats->_edenEvacuateRegionCount,
@@ -389,16 +397,44 @@ MM_SchedulingDelegate::partialGarbageCollectCompleted(MM_EnvironmentVLHGC *env,
389397
uintptr_t edenSurvivorCount = copyForwardStats->_edenSurvivorRegionCount;
390398
uintptr_t nonEdenSurvivorCount = copyForwardStats->_nonEdenSurvivorRegionCount;
391399

400+
/* Adjust estimated edenSurvivorCount and nonEdenSurvivorCount based on _copyBytesEden
401+
* due to fresh survivor region could be shared between eden evacuateRegions and age0 nonEden evacuateRegions.
402+
*/
403+
if (copyForwardStats->_copyBytesEden > 0) {
404+
uintptr_t neededEdenSurvivorCount = (copyForwardStats->_copyBytesEden + regionSize - 1) / regionSize;
405+
if (neededEdenSurvivorCount != edenSurvivorCount) {
406+
/* avoid nonEdenSurvivorCount underflow in case using age0 non-eden regions as survivors */
407+
if (neededEdenSurvivorCount > (nonEdenSurvivorCount + edenSurvivorCount)) {
408+
edenSurvivorCount += nonEdenSurvivorCount;
409+
nonEdenSurvivorCount = 0;
410+
} else {
411+
nonEdenSurvivorCount = nonEdenSurvivorCount + edenSurvivorCount - neededEdenSurvivorCount;
412+
edenSurvivorCount = neededEdenSurvivorCount;
413+
}
414+
}
415+
}
392416
/* estimate how many more regions we would have needed to avoid abort */
393417
Assert_MM_true( (0 == copyForwardStats->_scanBytesEden) || copyForwardStats->_aborted || (0 != copyForwardStats->_nonEvacuateRegionCount));
394418
Assert_MM_true( (0 == copyForwardStats->_scanBytesNonEden) || copyForwardStats->_aborted || (0 != copyForwardStats->_nonEvacuateRegionCount));
395419
edenSurvivorCount += (copyForwardStats->_scanBytesEden + regionSize - 1) / regionSize;
420+
if (edenSurvivorCount > edenCountBeforeCollect) {
421+
/* when abort is in progress, every Eden object scanned in abort-recovery contributes to _scannedBytes.
422+
* But an object that was partially copied before abort (the copy succeeded,
423+
* the forwarding pointer was installed) is also counted again during the abort scan of the source region.
424+
* So _scanBytesEden might be double-counted, then cause edenSurvivorCount is bigger than edenCountBeforeCollect.
425+
* And High survival + alignment/TLH waste(numa case and Hybrid noEvacuation, too)
426+
* for these cases, set edenSurvivorCount = edenCountBeforeCollect.
427+
*/
428+
edenSurvivorCount = edenCountBeforeCollect;
429+
}
396430
nonEdenSurvivorCount += (copyForwardStats->_scanBytesNonEden + regionSize - 1) / regionSize;
397431

398432
/* Eden count could be 0 in special case, after compaction if there is still no free region for scheduling eden(eden count = 0),
399433
will skip update Survival Rate */
400434
if (0 != edenCountBeforeCollect) {
435+
/* SurvivalRate should never be over 1.0 */
401436
double thisSurvivalRate = (double)edenSurvivorCount / (double)edenCountBeforeCollect;
437+
Assert_MM_true(1.0 >= thisSurvivalRate);
402438
updateSurvivalRatesAfterCopyForward(thisSurvivalRate, nonEdenSurvivorCount);
403439
}
404440

0 commit comments

Comments
 (0)