Fix exception "committed argument cannot be less than 0" #22150

LinHu2016 · 2025-06-26T15:45:36Z

Previous PR #17107 target
to prevent exception "committed argument cannot be less than 0" for
MemoryPool "balanced-reserved" during collecting memoryUsage at the end
of GC(balanced GC only), but there are two special cases haven't been
covered.

Fix case 1: maxHeapExpansionRegions could add on maxEdenChange and use
_extensions->globalVLHGCStats._heapSizingData.edenRegionChange to notify
heap resize to adjust heap size to match eden change, but it did not
consider the case which eden size is bigger than free memory, so update
_extensions->globalVLHGCStats._heapSizingData.edenRegionChange =
OMR_MIN(maxHeapExpansionRegions, edenChangeWithSurvivorHeadroom +
(intptr_t)(_edenRegionCount - freeRegions));
for keeping free memory >= eden size after heap resize.

Fix case 2: eden region can not be smaller than 1, when free region is 0
eden region size can be 0.

Add Assertion check to confirm free size >= eden size.

Signed-off-by: lhu linhu@ca.ibm.com

LinHu2016 · 2025-06-27T14:42:31Z

I have updated line
maxEdenChange = freeRegions - _edenRegionCount;
to

	if (freeRegions < _edenRegionCount) {
		maxEdenChange = freeRegions - _edenRegionCount;
	}

to avoid the original logic(before PR #17107) change.

For freeRegions >= _edenRegionCount case, keep maxEdenChange = 0

amicic · 2025-06-27T14:48:12Z

I have updated line maxEdenChange = freeRegions - _edenRegionCount; to
	if (freeRegions < _edenRegionCount) {
		maxEdenChange = freeRegions - _edenRegionCount;
	}
to avoid the original logic(before PR #17107) change.

I have a feeling it's ok to have maxEdenChange negative. Otherwise we may allow eden to stay unchanged (because maxEdenChange was left 0) in cases it must be contracted (since _edenRegionCount is less than freeRegions). You have to test this more....

keithc-ca · 2025-07-03T21:33:04Z

Please correct the spelling of "previous" in the description and the commit message.

amicic · 2025-07-07T18:06:08Z

runtime/gc_vlhgc/SchedulingDelegate.cpp

+	}
+
 	/* Eden will be stealing free regions from the entire heap, without telling the heap to grow.
 	 * Note: the eden sizing logic knows how much free memory is available in the heap, and knows to not grow too much.


Before we forget, but this part of the comment will not be true any more. We are changing the logic, we will not be stealing existing, but always growing. Let's remove it.

amicic · 2025-07-07T18:08:00Z

runtime/gc_vlhgc/SchedulingDelegate.cpp

-			_extensions->globalVLHGCStats._heapSizingData.edenRegionChange = OMR_MIN(maxHeapExpansionRegions, edenChangeWithSurvivorHeadroom + (intptr_t)(_edenRegionCount - freeRegions));
-		}
-	}
+	/* Eden will inform the total heap resizing logic, that it needs to change total heap size in order to maintain same "tenure" size */


This comment is too late now (since maxEdenChange calculation was move down) and even unrelated to maxEdenChange calculation. Move it up, just before the _heapSizingData.edenRegionChange calculation.

pshipton · 2025-07-24T12:53:48Z

@LinHu2016 is there any outlook for this being ready for review?

amicic · 2025-07-24T12:58:35Z

This is close to ready. Reviewing some testing results, if they are ok, probably only cosmetic code changes before merging.

amicic · 2025-07-24T13:04:49Z

runtime/gc_vlhgc/SchedulingDelegate.cpp

+		edenChangeWithSurvivorHeadroom = desiredEdenChangeSize + (intptr_t)ceil(((double)desiredEdenChangeSize * _edenSurvivalRateCopyForward));
+	} else if (0 > desiredEdenChangeSize) {
+		/* If eden is shrinking, only factor adjusting in survivor regions for total heap resizing when eden is not very small.
+		 * Factoring in survivor regions when eden is tiny can lead to some innacuracies, and reduce free non-eden regions, which may impact performance


we can remove this comment - we don't special treat very small eden

amicic · 2025-07-30T13:52:29Z

jenkins test extended.functional,extended.system,sanity.functional,sanity.openjdk,sanity.system amac,xLinux jdk21

runtime/gc_vlhgc/SchedulingDelegate.cpp

LinHu2016 · 2025-07-30T18:50:37Z

all above jjenkins test are passed,

updated above comment changes.

runtime/gc_vlhgc/SchedulingDelegate.hpp

runtime/gc_vlhgc/SchedulingDelegate.cpp

keithc-ca · 2025-07-31T16:00:38Z

Please update the commit message so sentences begin with a capital and end with a period (fix -> Fix, add -> Add).

keithc-ca · 2025-07-31T16:31:38Z

The final sentence in the commit message is missing a period.

amicic · 2025-08-01T09:48:09Z

runtime/gc_vlhgc/HeapRegionManagerVLHGC.cpp

 		snapshot->_totalRegionEdenSize = allocateEdenTotal;
 	}

+	Assert_MM_true(snapshot->_totalRegionReservedSize >= (snapshot->_totalRegionEdenSize - allocateEdenTotal));


Could we put a similar assert somewhere at GC end path, instead? Just before this is triggered (hence after all resing logic is complete). This will be tested only if MXBean is activated, but should always be.

Add
Assert_MM_true(((MM_GlobalAllocationManagerTarok *)_extensions->globalAllocationManager)->getFreeRegionCount() >= _edenRegionCount);
after calculateEdenSize(env); in heapReconfigured()
heapReconfigured() happens after Heap resize.

Could we do it even later, very close to when we trigger GC-end, so it covers other invocations of calculateEdenSize, too?

amicic · 2025-08-12T12:10:46Z

jenkins test extended.functional,extended.system,sanity.functional,sanity.openjdk,sanity.system amac,xLinux jdk21

Previous PR eclipse-openj9#17107 target to prevent exception "committed argument cannot be less than 0" for MemoryPool "balanced-reserved" during collecting memoryUsage at the end of GC(balanced GC only), but there are two special cases haven't been covered. Fix case 1: maxHeapExpansionRegions could add on maxEdenChange and use _extensions->globalVLHGCStats._heapSizingData.edenRegionChange to notify heap resize to adjust heap size to match eden change, but it did not consider the case which eden size is bigger than free memory, so update _extensions->globalVLHGCStats._heapSizingData.edenRegionChange = OMR_MIN(maxHeapExpansionRegions, edenChangeWithSurvivorHeadroom + (intptr_t)(_edenRegionCount - freeRegions)); for keeping free memory >= eden size after heap resize. Fix case 2: eden region can not be smaller than 1, when free region is 0 eden region size can be 0. Add Assertion checks to confirm free size >= eden size. Signed-off-by: lhu <linhu@ca.ibm.com>

LinHu2016 · 2025-08-12T16:31:31Z

amicic · 2025-08-12T19:36:19Z

jenkins test extended.functional,extended.system,sanity.functional,sanity.openjdk,sanity.system aix,win jdk21

LinHu2016 force-pushed the pmr_committedsizeisssue branch from b42da77 to 343f200 Compare June 26, 2025 16:56

dmitripivkine requested a review from amicic June 26, 2025 17:25

dmitripivkine added the comp:gc label Jun 26, 2025

LinHu2016 force-pushed the pmr_committedsizeisssue branch from 343f200 to 345cfcf Compare June 27, 2025 14:36

LinHu2016 force-pushed the pmr_committedsizeisssue branch 7 times, most recently from 329f56e to fe92579 Compare July 3, 2025 20:51

LinHu2016 force-pushed the pmr_committedsizeisssue branch 5 times, most recently from 1992225 to 0637fab Compare July 7, 2025 02:17

amicic reviewed Jul 7, 2025

View reviewed changes

LinHu2016 force-pushed the pmr_committedsizeisssue branch 2 times, most recently from 45eb219 to dc057e3 Compare July 8, 2025 14:58

LinHu2016 force-pushed the pmr_committedsizeisssue branch 4 times, most recently from 95be277 to fc4d337 Compare July 21, 2025 14:41

amicic reviewed Jul 24, 2025

View reviewed changes

LinHu2016 force-pushed the pmr_committedsizeisssue branch 3 times, most recently from 5d0c204 to 32cdc05 Compare July 30, 2025 02:13

keithc-ca requested changes Jul 30, 2025

View reviewed changes

runtime/gc_vlhgc/SchedulingDelegate.cpp Outdated Show resolved Hide resolved

runtime/gc_vlhgc/SchedulingDelegate.cpp Outdated Show resolved Hide resolved

LinHu2016 force-pushed the pmr_committedsizeisssue branch 3 times, most recently from 4f190a2 to 0842584 Compare July 30, 2025 18:47

keithc-ca requested changes Jul 30, 2025

View reviewed changes

runtime/gc_vlhgc/SchedulingDelegate.hpp Outdated Show resolved Hide resolved

runtime/gc_vlhgc/SchedulingDelegate.cpp Outdated Show resolved Hide resolved

runtime/gc_vlhgc/SchedulingDelegate.cpp Outdated Show resolved Hide resolved

LinHu2016 force-pushed the pmr_committedsizeisssue branch from 0842584 to 5338b7c Compare July 31, 2025 14:29

keithc-ca requested changes Jul 31, 2025

View reviewed changes

runtime/gc_vlhgc/SchedulingDelegate.cpp Outdated Show resolved Hide resolved

LinHu2016 force-pushed the pmr_committedsizeisssue branch from 5338b7c to c7f821c Compare July 31, 2025 16:25

LinHu2016 force-pushed the pmr_committedsizeisssue branch 2 times, most recently from 574e1de to d0061e7 Compare July 31, 2025 17:06

keithc-ca approved these changes Jul 31, 2025

View reviewed changes

amicic reviewed Aug 1, 2025

View reviewed changes

LinHu2016 force-pushed the pmr_committedsizeisssue branch from d0061e7 to b7039c1 Compare August 1, 2025 14:43

LinHu2016 force-pushed the pmr_committedsizeisssue branch from b7039c1 to 26753c8 Compare August 11, 2025 18:28

LinHu2016 force-pushed the pmr_committedsizeisssue branch from 26753c8 to 11f5a78 Compare August 12, 2025 16:30

amicic approved these changes Aug 13, 2025

View reviewed changes

amicic merged commit a075ec5 into eclipse-openj9:master Aug 13, 2025
15 checks passed

Fix exception "committed argument cannot be less than 0" #22150

Fix exception "committed argument cannot be less than 0" #22150

Uh oh!

Conversation

LinHu2016 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinHu2016 commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amicic commented Jun 27, 2025

Uh oh!

keithc-ca commented Jul 3, 2025

Uh oh!

amicic Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

amicic Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

pshipton commented Jul 24, 2025

Uh oh!

amicic commented Jul 24, 2025

Uh oh!

amicic Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

amicic commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

LinHu2016 commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithc-ca commented Jul 31, 2025

Uh oh!

keithc-ca commented Jul 31, 2025

Uh oh!

amicic Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

LinHu2016 Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

amicic Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

amicic commented Aug 12, 2025

Uh oh!

LinHu2016 commented Aug 12, 2025

Uh oh!

amicic commented Aug 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LinHu2016 commented Jun 26, 2025 •

edited

Loading

LinHu2016 commented Jun 27, 2025 •

edited

Loading