New eden and heap sizing logic in Balanced GC #12043

cedrichansen · 2021-02-24T17:12:47Z

This change introduces 2 different major components to balanced gc, which are working together.

---------Change 1: New heap sizing logic in balanced gc
The old heap sizing logic, relied primarily on free memory %, in relation to -Xminf and -Xmaxf to decide whether or not the heap should shrink/expand.
If free memory was within the acceptable boundary, the heap sizing logic would then look at gc cpu% to decide whether to expand/contract.

The new logic (which is included in this commit), aims to take a more hybrid approach, where TENURE free memory % and GMP cpu % are working together, to determine whether or not the heap needs to change.
The function calculateCurrentHybridHeapOverhead determines what the current "hybrid overhead" of the heap/gc is (blend of gc cpu % and free memory %).
If this "hybrid overhead" is above -Xmaxt (default to 5%), the heap will expand. Conversely, if the "hybrid overhead" is below -Xmint (2% default), the heap will shrink.

--------Change 2: New eden sizing logic in balanced

The current logic for eden sizing is fairly simple - By default, eden is taken to be 25% of the heap - unless specified otherwise by -Xmn/-Xmns/-Xmnx

The new logic, aims to make use of a few heuristics to improve overall PGC overheaad (% of time being active relative to total time), while trying to respect the newly introduced, tarokTargetMaxPauseTime.
Eden sizing logic now adheres to the following high level concepts:

If the heap is not fully expanded ( that is current heap size <= -Xsoftmx/-Xmx), then eden will increase by 5% if PGC overhead and pgc avg time blend, is above
dnssExpectedTimeRatioMinimum (5% default). Conversely, eden will shrink by 5% if the hybrid blend of pgc overhead and pgc time, falls below dnssExpectedTimeRatioMaximum (2% default). When the heap is not fully expanded, any changes in eden size, will result in the total heap to be resized by the same amount, in order to keep non-eden size the same.
If the heap is fully expanded, a prediction will be made as to which eden size will produce the lowest total GC cpu overhead, while attempting to respect tarokTargetMaxPauseTime, given the constraints imposed by the heap (free memory, GMP cost, etc.
This prediction accounts for PGC average time/intervals, GMP time/intervals, free Tenure space, and survival ratio of objects being copied out of eden, in order to determine the "best" eden size.
When the heap is fully expanded, and eden grows, it will "steal" free regions from "tenure"/non-eden.

Note: The following command line options are now supported by balanced gc policy as part of this change.
- -Xgc:dnssExpectedTimeRatioMinimum -> sets min expected % of time spent in pgc pauses (default 2% for balanced)
- -Xgc:dnssExpectedTimeRatioMaximum -> sets max expected % of time spent in pgc pauses (default 5% for balanced)
- -Xgc:targetPausetime -> sets target (tarokTargetMaxPauseTime defaults to 200ms in balanced)

Depends on: eclipse-omr/omr#5825

Signed-off-by: Cedric Hansen cedric.hansen@ibm.com

cedrichansen · 2021-02-24T17:19:59Z

@amicic FYI

amicic · 2021-02-24T20:38:15Z

runtime/gc_vlhgc/MemorySubSpaceTarok.cpp

+		sizeChange = calculateContractionSize(env, allocDescription, _systemGC, true);
+	}
+
+	if (0 == sizeChange && hybridHeapScore >= (double)_extensions->heapContractionGCTimeThreshold) {


As per coding standards, we put extra oval brackets and don't rely on operator priorities:

((0 == sizeChange) && (hybridHeapScore >= (double)_extensions->heapContractionGCTimeThreshold))

Please go through all the changes and add them when necessary. For this pass I'll ignore the rest of missing ones.

amicic · 2021-02-24T20:47:27Z

One general comment: if you think you touched a significant part of a file (50% would suffice), then convert J9 specific integer types like UDATA, I_32 etc... into standard types uintptr_t, int32_t etc in the whole file.

amicic · 2021-02-24T20:48:45Z

Please create an Issue, and move most of PR/commit comments there.

cedrichansen · 2021-02-25T17:48:31Z

This PR addresses #12054

cedrichansen · 2021-03-01T18:32:24Z

The latest 2 commits include the following changes:

Fixed some "if" statements conditions to adhere to coding standards
Changed j9 specific integer types, into standard integer types for files which saw significant modifications

dmitripivkine · 2021-03-01T19:00:44Z

runtime/gc_base/GCExtensions.cpp

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 1991, 2020 IBM Corp. and others
+ * Copyright (c) 1991, 2021 IBM Corp. and others


This file after all has not been changed. Would you please just exclude it from change set without modification of copyrights data?

dmitripivkine · 2021-03-01T19:02:49Z

runtime/gc_modron_startup/mmparseXgc.cpp


-		goto _exit;
-	}
+		#if defined(J9VM_GC_REALTIME)


all preprocessor lines should start column 0

runtime/gc_vlhgc/GlobalMarkingScheme.cpp

dmitripivkine · 2021-03-01T19:29:52Z

runtime/gc_vlhgc/MemorySubSpaceTarok.hpp

@@ -60,6 +60,8 @@ class MM_MemorySubSpaceTarok : public MM_MemorySubSpace

 	MM_HeapRegionManager *_heapRegionManager;	/**< Stored so that we can resolve the table descriptor for given addresses when asked for a pool corresponding to a specific address */
 	MM_LightweightNonReentrantLock _expandLock; /**< Most of the common expand code is not multi-threaded safe (since it used in standard collectors on alloc path fail path which is single threaded)  */
+	double _lastObservedGcPercentage; /**< The most recently observed GC percentage (time gc is active / time gc is not active) */
+	U_64 _previouslyObservedPGCCount; /**< The count of observed PGC's since the last GMP cycle - Used to determine if heap resize occured after a system/global gc */


if you change variable types to standard in correspondent .cpp file, so it make sense to change types in .hpp as a pair

cedrichansen · 2021-03-01T20:43:05Z

@dmitripivkine PR comments thus far have been addressed. These includes:

Fixing multiline comments to adhere to style standards
move preprocessor instruction to start of the line
making MemorySubspaceTarok.hpp integer types consistent with .cpp integer types
removed copyright update for untouched file -> GCextensionsBase.cpp

amicic · 2021-03-01T20:46:03Z

runtime/gc_vlhgc/MemorySubSpaceTarok.cpp

+}	
+
+double
+MM_MemorySubSpaceTarok::calculateHybridHeapOverhead(MM_EnvironmentBase *env, IDATA heapChange)


there are a few of IDATAs that should be converted to intptr_t

amicic · 2021-03-01T20:46:50Z

runtime/gc_vlhgc/MemorySubSpaceTarok.cpp

+		Trc_MM_MemorySubSpaceTarok_calculateHybridHeapOverhead(env->getLanguageVMThread(), gcPercentage, freeMemComponant);
+	}
+
+	return 	(gcPercentage * gcOverheadWeight) + (freeMemComponant * (1 - gcOverheadWeight)); 


you can still use weightedAverage here

amicic · 2021-03-01T20:47:11Z

runtime/gc_vlhgc/SchedulingDelegate.cpp

+	}
+
+	double recentOverhead = (double)(_historicalPartialGCTime * 1000)/_averagePgcInterval;
+	_partialGcOverhead = (double)MM_Math::weightedAverage((float)_partialGcOverhead, (float)recentOverhead, 0.5);


introduce weightedAverage for double

eclipse-omr/omr@0aa8abb adds MM_Math::weightedAverage for doubles (this is a second commit on linked OMR PR)

amicic · 2021-03-01T20:49:32Z

runtime/gc_vlhgc/MemorySubSpaceTarok.cpp

+double 
+MM_MemorySubSpaceTarok::calculateGcPctForHeapChange(MM_EnvironmentBase *env, IDATA heapSizeChange)
+{
+	MM_EnvironmentVLHGC *envVLHGC = (MM_EnvironmentVLHGC *)env;


we typically name the one from the param envBase, and then this one just env

In this file, I have seen it called envVLHGC, but am happy to refactor.

See below for example https://github.com/eclipse/openj9/blob/8394cffecaf5ea67e60c837a3b8f27e8c7d5c290/runtime/gc_vlhgc/MemorySubSpaceTarok.cpp#L447

You are right the existing pattern is predominant. I like what I proposed because the sites that use them will have a bit less characters, but it's not a strong argument. We can leave it as is - up to you.

amicic · 2021-03-01T20:51:20Z

runtime/gc_vlhgc/MemorySubSpaceTarok.cpp

+MM_MemorySubSpaceTarok::getHeapSizeWithinBounds(MM_EnvironmentBase *env)
+{
+	double currentHybridHeapScore = calculateCurrentHybridHeapOverhead(env);
+	uintptr_t reccomendedHeapSize = getActiveMemorySize();


spelling: recommended

cedrichansen · 2021-03-01T21:54:44Z

@amicic PR comments have been addressed. These include:

Converting IDATA -> intptr_t in MemorySubSpaceTarok
Fix spelling mistake 'reccomended' -> 'recommended'
using weightedAverage which takes double instead of float

I left out the change related to envVLHGC since some parts of the file in question use the same nomenclature I used for that same cast, but I can gladly change it if need be

cedrichansen · 2021-03-03T15:23:15Z

runtime/gc_vlhgc/SchedulingDelegate.cpp

+	if ((x1 < x2) && (y1 < y2)) {
+		double timeDiff = y1 - y2;
+		double edenSizeRatio = ((x1 + 1.0) / (x2 + 1.0));
+		_pgcTimeIncreasePerEdenRegionFactor = pow(edenSizeRatio, (1.0/timeDiff));


Just a note for reviewers - this _pgcTimeIncreasePerEdenRegionFactor should not be a weighted average.

Rationale - _pgcTimeIncreasePerEdenRegionFactor is not "linear" in the sense that the difference between 1.1 and 1.2, is not "the same" as 1.2 to 1.3, since this equation is finding the logarithmic fit between the 2 points.
That being said, using a weighted average, is not really a good idea. Additionally, _historicalPartialGCTime is being used for this calculation, which is already a moving average, so no need to make _pgcTimeIncreasePerEdenRegionFactor a weighted average as well.

This is probably ok for now, but there are still some assumed consts here, which would fit for some workloads/current eden sizes and not for others. Specifically, A (assumed 1.0) here would could vary:

pow(edenSizeRatio, ( A /timeDiff));

How I would have done it (in theory anyway) is to remember last few (5-10) of (size, time) pairs and do a linear regression. That would give you first derivative (slope of the whatever the real type of the curve is) in the neighborhood of recent eden sizes. Indeed this is what we need, since we are typically doing incremental resizing (<10%), not that far from the recent past samples. Basically, a very simple machine learning.

That approach would get rid of assumptions like minimumPgcTime, factor A etc,
but it would be a lot more calculations and early estimates (during startup/rampup) would be fairly inaccurate.

Here the 1.0, in pow(edenSizeRatio, ( 1.0 /timeDiff)); is a workaround to represent nthroot in c++. The actual formula is the following

Where z in the image, is _pgcTimeIncreasePerEdenRegionFactor in the code. That being said, I don't really see the 1.0 changing, unless we change the formula (which might be an option).

As for the suggested improvement, I agree that this would be a good refinement, and I think we had briefly spoken about doing something along those lines, but as you mention, it adds more complexity/calculations. Up to you if we tackle that refinement in this PR, or at a later point.

With New eden and heap sizing logic in Balanced GC(introduced in eclipse-openj9#12043), maximum Eden memorypool size could reach to 75% maximum heap size or the size specified in -Xmn/-Xmnx minimum Eden size is 2 * regions size or the size specified in -Xmn/-Xmns

eclipse-openj9/openj9#12043 introduced a warning when compiled to32 bit platforms. This commit adds the appropriate cast to fix the warning. Signed-off-by: Cedric Hansen <cedric.hansen@ibm.com>

With New eden and heap sizing logic in Balanced GC(introduced in eclipse-openj9/openj9#12043), maximum Eden memorypool size could reach to 75% maximum heap size or the size specified in -Xmn/-Xmnx minimum Eden size is 2 * regions size or the size specified in -Xmn/-Xmns

eclipse-openj9/openj9#12043 will add support for the options mentioned above, for the Balanced GC policy. This PR contains the documentation changes required to reflect this added support. closes eclipse-openj9#836 Signed-off-by: Cedric Hansen <cedric.hansen@ibm.com>

eclipse-openj9/openj9#12043 will add support for the options mentioned above, for the Balanced GC policy. This PR contains the documentation changes required to reflect this new behaviour. closes eclipse-openj9#834 Signed-off-by: Cedric Hansen <cedric.hansen@ibm.com>

…ioMinimum` support for Balanced GC eclipse-openj9/openj9#12043 will add support for the options mentionned above, for the Balanced GC policy. This PR contains the documentation changes required to reflect this added support. closes eclipse-openj9#836 Signed-off-by: Cedric Hansen <cedric.hansen@ibm.com>

eclipse-openj9/openj9#12043 will add support for the options mentioned above, for the Balanced GC policy. This PR contains the documentation changes required to reflect this added support. closes eclipse-openj9#836 Signed-off-by: Cedric Hansen <cedric.hansen@ibm.com>

eclipse-openj9/openj9#12043 will add support for the option mentioned above, for the Balanced GC policy. This PR contains the documentation changes required to reflect this added support. closes eclipse-openj9#833 Signed-off-by: Cedric Hansen <cedric.hansen@ibm.com>

eclipse-openj9/openj9#12043 will add support for the options mentioned above, for the Balanced GC policy. This PR contains the documentation changes required to reflect this new behaviour. closes eclipse-openj9#834 Signed-off-by: Cedric Hansen <cedric.hansen@ibm.com>

cedrichansen force-pushed the balancedHeapHeuristics branch from 126aed1 to 09108ab Compare February 24, 2021 17:18

cedrichansen force-pushed the balancedHeapHeuristics branch 3 times, most recently from 151df40 to 32ea4e0 Compare February 24, 2021 17:33

amicic added the comp:gc label Feb 24, 2021

amicic self-requested a review February 24, 2021 20:30

amicic reviewed Feb 24, 2021

View reviewed changes

cedrichansen force-pushed the balancedHeapHeuristics branch 3 times, most recently from 77807e9 to bdf285b Compare February 26, 2021 20:56

cedrichansen mentioned this pull request Feb 26, 2021

Rework Balanced GC heap and eden sizing logic #12054

Open

cedrichansen force-pushed the balancedHeapHeuristics branch from 939936d to 8fefc39 Compare March 1, 2021 16:59

dmitripivkine reviewed Mar 1, 2021

View reviewed changes

cedrichansen force-pushed the balancedHeapHeuristics branch from a45c004 to ad1cf7e Compare March 1, 2021 19:05

dmitripivkine reviewed Mar 1, 2021

View reviewed changes

runtime/gc_vlhgc/GlobalMarkingScheme.cpp Show resolved Hide resolved

dmitripivkine reviewed Mar 1, 2021

View reviewed changes

amicic reviewed Mar 1, 2021

View reviewed changes

cedrichansen commented Mar 3, 2021

View reviewed changes

LinHu2016 mentioned this pull request Sep 24, 2021

Update maximum EdenSize reporting #13567

Merged

pshipton mentioned this pull request Oct 1, 2021

JDK8 Windows IA32 : FVT_Serviceability.diagnostics.oom.Mode131 - ERROR: CLASSES/END Section missing from Javadump #13608

Open

pshipton added the depends:doc label Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New eden and heap sizing logic in Balanced GC #12043

New eden and heap sizing logic in Balanced GC #12043

cedrichansen commented Feb 24, 2021 •

edited

Loading

cedrichansen commented Feb 24, 2021

amicic Feb 24, 2021

amicic commented Feb 24, 2021

amicic commented Feb 24, 2021

cedrichansen commented Feb 25, 2021

cedrichansen commented Mar 1, 2021

dmitripivkine Mar 1, 2021

cedrichansen Mar 1, 2021

dmitripivkine Mar 1, 2021

dmitripivkine Mar 1, 2021 •

edited

Loading

cedrichansen commented Mar 1, 2021

amicic Mar 1, 2021

amicic Mar 1, 2021

amicic Mar 1, 2021

cedrichansen Mar 1, 2021 •

edited

Loading

amicic Mar 1, 2021

cedrichansen Mar 1, 2021 •

edited

Loading

amicic Mar 12, 2021

amicic Mar 1, 2021

cedrichansen commented Mar 1, 2021

cedrichansen Mar 3, 2021

amicic Mar 15, 2021 •

edited

Loading

cedrichansen Mar 15, 2021 •

edited

Loading

New eden and heap sizing logic in Balanced GC #12043

New eden and heap sizing logic in Balanced GC #12043

Conversation

cedrichansen commented Feb 24, 2021 • edited Loading

cedrichansen commented Feb 24, 2021

Choose a reason for hiding this comment

amicic commented Feb 24, 2021

amicic commented Feb 24, 2021

cedrichansen commented Feb 25, 2021

cedrichansen commented Mar 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

cedrichansen commented Mar 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedrichansen Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedrichansen Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedrichansen commented Mar 1, 2021

Choose a reason for hiding this comment

amicic Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

cedrichansen Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

cedrichansen commented Feb 24, 2021 •

edited

Loading

dmitripivkine Mar 1, 2021 •

edited

Loading

cedrichansen Mar 1, 2021 •

edited

Loading

cedrichansen Mar 1, 2021 •

edited

Loading

amicic Mar 15, 2021 •

edited

Loading

cedrichansen Mar 15, 2021 •

edited

Loading