Skip to content

Commit aee53e8

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. 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. Fix2/3:Use edenEvacuateBytes(=edenCountBeforeCollect * regionSize) and edenSurvivorBytes(= copyForwardStats->_copyBytesEden) instead of edenSurvivorCount and nonEdenSurvivorCount to prevent Semantic mismatch and Over-counting issues. TODO: To improve the accuracy of edenSurvivalRate, edenEvacuateBytes should exclude the free bytes remaining in evacuated Eden regions (region->getMemoryPool()->getFreeMemoryAndDarkMatterBytes()), as they do not represent evacuated live data. 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. this can push edenSurvivorBytes above edenEvacuateBytes. Fix: cap edenSurvivorBytes at edenEvacuateBytes and add an assertion that thisSurvivalRate <= 1.0. Signed-off-by: lhu <linhu@ca.ibm.com>
1 parent eb3e17a commit aee53e8

1 file changed

Lines changed: 31 additions & 9 deletions

File tree

runtime/gc_vlhgc/SchedulingDelegate.cpp

Lines changed: 31 additions & 9 deletions
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,
@@ -384,21 +392,35 @@ MM_SchedulingDelegate::partialGarbageCollectCompleted(MM_EnvironmentVLHGC *env,
384392

385393
if (env->_cycleState->_shouldRunCopyForward) {
386394
uintptr_t regionSize = _regionManager->getRegionSize();
387-
388-
/* count the number of survivor regions allocated specifically to support Eden survivors */
389-
uintptr_t edenSurvivorCount = copyForwardStats->_edenSurvivorRegionCount;
395+
uintptr_t edenEvacuateBytes = edenCountBeforeCollect * regionSize;
396+
/* _copyDiscardBytesEden is excluded from edenSurvivorBytes because it is related with
397+
* a proportion of non-Eden age-0 survivor objects, making it unsuitable for
398+
* estimating Eden survivor bytes.
399+
*/
400+
uintptr_t edenSurvivorBytes = copyForwardStats->_copyBytesEden;
401+
/* nonEdenSurvivorCount non age0 fresh survivor region count (excludes any survivor regions for age0 non-Eden evacuate regions) */
390402
uintptr_t nonEdenSurvivorCount = copyForwardStats->_nonEdenSurvivorRegionCount;
391403

392404
/* estimate how many more regions we would have needed to avoid abort */
393405
Assert_MM_true( (0 == copyForwardStats->_scanBytesEden) || copyForwardStats->_aborted || (0 != copyForwardStats->_nonEvacuateRegionCount));
394406
Assert_MM_true( (0 == copyForwardStats->_scanBytesNonEden) || copyForwardStats->_aborted || (0 != copyForwardStats->_nonEvacuateRegionCount));
395-
edenSurvivorCount += (copyForwardStats->_scanBytesEden + regionSize - 1) / regionSize;
407+
edenSurvivorBytes += copyForwardStats->_scanBytesEden;
408+
if (edenSurvivorBytes > edenEvacuateBytes) {
409+
Assert_MM_true(copyForwardStats->_aborted);
410+
/* when abort is in progress, every Eden object scanned in abort-recovery contributes to _scannedBytes.
411+
* But an object that was partially copied before abort (the copy succeeded,
412+
* the forwarding pointer was installed) is also counted again during the abort scan of the source region.
413+
* So _scanBytesEden might be double-counted, then cause edenSurvivorBytes is bigger than edenEvacuateBytes.
414+
* for the case, set edenSurvivorBytes = edenEvacuateBytes.
415+
*/
416+
edenSurvivorBytes = edenEvacuateBytes;
417+
}
396418
nonEdenSurvivorCount += (copyForwardStats->_scanBytesNonEden + regionSize - 1) / regionSize;
397419

398-
/* Eden count could be 0 in special case, after compaction if there is still no free region for scheduling eden(eden count = 0),
399-
will skip update Survival Rate */
400-
if (0 != edenCountBeforeCollect) {
401-
double thisSurvivalRate = (double)edenSurvivorCount / (double)edenCountBeforeCollect;
420+
if (0 != edenEvacuateBytes) {
421+
/* SurvivalRate should never be over 1.0 */
422+
double thisSurvivalRate = (double)edenSurvivorBytes / (double)edenEvacuateBytes;
423+
Assert_MM_true(1.0 >= thisSurvivalRate);
402424
updateSurvivalRatesAfterCopyForward(thisSurvivalRate, nonEdenSurvivorCount);
403425
}
404426

0 commit comments

Comments
 (0)