Specify heap limits as percentage of total memory #1699

ashu-mehra · 2018-04-16T16:50:32Z

Add options for specifying heap limits in terms of percentage of
total memory available to the JVM.
Following options are added:

-XX:MaxRAMPercentage=N where 0 <= N <= 100.0
This option specifies maximum heap size. If -Xmx is specified, this
option is ignored.
-XX:InitialRAMPercentage=N where 0 <= N <= 100.0
This option specifies initial heap size. If -Xms is specified, this
option is ignored.

Closes: #1428

Signed-off-by: Ashutosh Mehra asmehra1@in.ibm.com

pshipton · 2018-04-16T17:16:34Z

The description should contain -XX:MaxRAMPercentage instead of -XX:MaxRAMPercent, same for -XX:InitialRAMPercentage

ashu-mehra · 2018-04-16T17:42:05Z

The description should contain -XX:MaxRAMPercentage instead of -XX:MaxRAMPercent, same for -XX:InitialRAMPercentage

Updated the comment, as well as the commit message.

dmitripivkine · 2018-04-16T18:33:20Z

Discussion question: Do we need full (signed) double parsing for this particular case? If we do it unsigned would be no underflow case to handle. If we need to have general double parsing I am ok with it

dmitripivkine · 2018-04-16T18:38:35Z

runtime/gc_modron_startup/mmparse.cpp

+					extensions->memoryMax = (usableMemory / 100) * maxRAMPercent;
+
+					/* Hack table to appear that -Xmx was specified */
+					memoryParameters[opt_Xmx] = index;


I don't like this solution. I believe the option should be parsed and stored in gc extensions. MemoryMax and initialMemorySize should be adjusted at startup properly. I would try to avoid any decisions in parse module.

dmitripivkine · 2018-04-16T18:39:26Z

runtime/gc_modron_startup/mmparse.cpp

+					extensions->initialMemorySize = (usableMemory / 100) * initialRAMPercent;
+
+					/* Hack table to appear that -Xms was specified */
+					memoryParameters[opt_Xms] = index;


the same as comment for MemoryMax

ashu-mehra · 2018-04-24T13:02:42Z

Do we need full (signed) double parsing for this particular case? If we do it unsigned would be no underflow case to handle. If we need to have general double parsing I am ok with it

I have updated the code to avoid signed parsing.

@dmitripivkine @DanHeidinga can you please review the changes.

ashu-mehra · 2018-04-24T13:03:13Z

I have removed WIP from the subject line.

dmitripivkine · 2018-04-24T13:36:05Z

runtime/gc_modron_startup/mminit.cpp

+adjustXmxXmsUsingRAMPercent(MM_GCExtensions *extensions, IDATA *memoryParameters)
+{
+	OMRPORT_ACCESS_FROM_OMRVM(extensions->getOmrVM());
+	uint64_t usableMemory = omrsysinfo_get_addressable_physical_memory();


This function is called already in MM_GCExtensions::computeDefaultMaxHeap. In my opinion you should integrate handling of maxRAMPercent and initialRAMPercent in that code. Please not modify memoryParameters[opt_Xms] and memoryParameters[opt_Xmx]. Let them keep original command line option values if any.

In my opinion you should integrate handling of maxRAMPercent and initialRAMPercent in that code.

But computeDefaultMaxHeap() is called even before we parse gc options in gcParseCommandLineAndInitializeWithValues().

Please not modify memoryParameters[opt_Xms] and memoryParameters[opt_Xmx].

Reason for modifying memoryParameters[opt_Xms] is the code in gcInitializeCalculatedValues() which sets initialMemorySize to default value if memoryParameters[opt_Xms] is not set.
So If I set initialMemorySize using initialRAMPercentage but don't update memoryParameters[opt_Xms], it will get overwritten to default value in gcInitializeCalculatedValues().

Right, I see. What if we call omrsysinfo_get_addressable_physical_memory() once in MM_GCExtensionsBase before computeDefaultMaxHeap() and store it there. So there would be no need to call it again. Use stored value in MM_GCExtensionsBase::computeDefaultMaxHeap() and MM_GCExtensions::computeDefaultMaxHeap() implementations as well as here.
Modification of memoryParameters for opt_Xms and opt_Xmx is not my preferable way (it is a hack). However considering complicate messy logic in mminit.c related with memory parameters adjustments (needs to be cleaned up obviously but not in this PR) I would except this. Do we need to create adjustXmxXmsUsingRAMPercent() function? I would just add these two "if" statements directly instead (as far as there is no other consumers for this logic).

I agree, we should store omrsysinfo_get_addressable_physical_memory() in a MM_GCExtensionsBase.

dmitripivkine · 2018-04-24T18:58:50Z

runtime/gc_modron_startup/mmparse.cpp

+	memoryParameters[opt_maxRAMPercent] = index;
+	if (memoryParameters[opt_maxRAMPercent] != -1) {
+		if (extensions->maxRAMPercent > 100) {
+			result = OPTION_OVERFLOW;


Should we create something like J9NLS_GC_OPTIONS_DOUBLE_OUT_OF_RANGE (using existing J9NLS_GC_OPTIONS_INTEGER_OUT_OF_RANGE as example)? It would be more informative

dmitripivkine · 2018-05-08T18:43:47Z

runtime/util_core/j9argscan.c

+
+	*result = strtod(*scan_start, &endPtr);
+	if (ERANGE == errno) {
+		if (0.0 == *result) {


You can not do it according spec: http://www.cplusplus.com/reference/cstdlib/strtod/
in case of ERANGE if return is not equal +/- HUGE_VAL (not overflow) this is underflow. The output value in this case is "value whose magnitude is no greater than the smallest normalized positive number" (not necessarily 0.0). If you want to handle it as zero *result should be set 0.0 before return with rc 0.

As per man page of strtod:
If the correct value would cause overflow, plus or minus HUGE_VAL (HUGE_VALF, HUGE_VALL) is returned (according to the sign of the value), and ERANGE is stored in errno. If the correct value would cause underflow, zero is returned and ERANGE is stored in errno.
Based on that I consider it as underflow if *return is 0.0.

From: http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtod.html
The Open Group Base Specifications Issue 7, 2018 edition
IEEE Std 1003.1-2017 (Revision of IEEE Std 1003.1-2008)

"If the correct value would cause an underflow, a value whose magnitude is no greater than the smallest normalized positive number in the return type shall be returned and errno set to [ERANGE]."

dmitripivkine · 2018-05-08T19:25:07Z

runtime/gc_modron_startup/mminit.cpp

+	if ((-1 == memoryParameterTable[opt_Xms]) && (-1 != memoryParameterTable[opt_initialRAMPercent])) {
+		extensions->initialMemorySize = (uintptr_t)(((double)extensions->usablePhysicalMemory / 100.0) * extensions->initialRAMPercent);
+		/* Update memory parameter table to appear that -Xms was specified */
+		memoryParameterTable[opt_Xms] = memoryParameterTable[opt_initialRAMPercent];


This means that in case of error related to out-of-range case (<1m for example) output message would mention -Xms option. This fact should be documented at least. Ideally error message should refer to real option whatever it is.

How about updating the message to say " -Xmx (as set by -XX:InitialRAMPercentage)" when -XX:MaxRAMPercentage is being used to set max heap size. Some examples of updated messages:

Option too large: -Xmx (as set by -XX:InitialRAMPercentage) is too large

-Xmx (as set by -XX:InitialRAMPercentage) too small, must be at least 1 Mbytes

I can add a function that returns "-Xmx" or "-Xmx (as set by -XX:InitialRAMPercentage) " based on the option being used:

const char * displayXmsOrInitialRAMPercentage(IDATA* memoryParameters) { if ((-1 == memoryParameters[opt_initialRAMPercent]) && (memoryParameters[opt_Xms] == memoryParameters[opt_initialRAMPercent]) ) { return "-Xms (as set by -XX:InitialRAMPercentage)"; } else { return "-Xms"; } }

and we can use this function wherever we are currently displaying "-Xmx".
Does that look fine?

yes, something like this

dmitripivkine · 2018-05-08T19:25:36Z

runtime/gc_modron_startup/mminit.cpp

+	if ((-1 == memoryParameterTable[opt_Xmx]) && (-1 != memoryParameterTable[opt_maxRAMPercent])) {
+		extensions->memoryMax = (uintptr_t)(((double)extensions->usablePhysicalMemory / 100.0) * extensions->maxRAMPercent);
+		/* Update memory parameter table to appear that -Xmx was specified */
+		memoryParameterTable[opt_Xmx] = memoryParameterTable[opt_maxRAMPercent];


This means that in case of error related to out-of-range case output message would mention -Xmx option. This fact should be documented at least. Ideally error message should refer to real option whatever it is.

ashu-mehra · 2018-05-14T06:13:47Z

@dmitripivkine I updated the code as suggested. Can you please review it.

dmitripivkine · 2018-05-14T14:16:01Z

runtime/util/vmargs.c

+		option = MAPPING_MAPNAME(j9vm_args, element);
+	}
+
+	return j9vm_args->actualVMArgs->options[element].optionString + strlen(option);


Is check for (NULL == option) missed there? I believe strlen(NULL) will crash, is not it?
getOptionValue() can return NULL in this case and caller might check it to return OPTION_ERROR

I added check for (NULL == option).
The current users of getOptionValue() don't need to check for NULL return value as we already ensure that optionName is not NULL.

dmitripivkine · 2018-05-14T18:14:06Z

runtime/util/vmargs.c

-					if (*valuesBuffer) {
-						scanStart = &(j9vm_args->actualVMArgs->options[element].optionString[ strlen(*valuesBuffer) ]);
-					} else {
-						return OPTION_ERROR;


For reviewers: this "else" is not necessary because of check lines 363-364, it is removed in new code

dmitripivkine

Looks good to me

dmitripivkine · 2018-05-14T18:34:37Z

@DanHeidinga Would you please do final review?

dmitripivkine · 2018-05-14T18:35:33Z

Also this feature should be documented.

dmitripivkine · 2018-05-14T18:41:56Z

Note: Currently Java 8 does not take the amount of RAM to consideration to calculate -Xmx default. It is applicable for Java 9 and higher. The -XX:MaxRAMPercentage option however is applicable for Java 8 so it introduces a way to set -Xmx as a percentage of machine's RAM.

DanHeidinga · 2018-05-22T15:02:50Z

runtime/util_core/j9argscan.c

+	if (ERANGE == errno) {
+		if ((HUGE_VAL == *result) || (-HUGE_VAL == *result)) {
+			/* overflow */
+			return 2;


Can this return OPTION_OVERFLOW instead of a magic number? Also, please add this to the function comment.

DanHeidinga · 2018-05-22T15:03:24Z

runtime/util_core/j9argscan.c

+		}
+	} else if ((0.0 == *result) && (endPtr == *scan_start)) {
+		/* no conversion */
+		return 1;


Can this return OPTION_MALFORMED? Also worth documenting in the function comment.

DanHeidinga · 2018-05-22T15:05:22Z

runtime/util/vmargs.c

@@ -629,6 +639,23 @@ optionValueOperations(J9PortLibrary *portLibrary, J9VMInitArgs* j9vm_args, IDATA
 	return OPTION_OK;
 }

+static char * 
+getOptionValue(J9VMInitArgs* j9vm_args, IDATA element, const char *optionName)


getOptionValue -> getStartOfOption? startOfOptionValue? I'm not a fan of this function name but the alternatives I've suggested aren't great either.

Due to lack of a better name, lets go with getStartOfOptionValue.

DanHeidinga · 2018-05-22T15:07:15Z

runtime/util/vmargs.c

+		option = MAPPING_MAPNAME(j9vm_args, element);
+	}
+
+	if (NULL != option) {


Can this be a trace assert that option is not null? A null optionName with no mapping is a programming error.

Add options for specifying heap limits in terms of percentage of total memory available to the JVM. Following options are added: 1. -XX:MaxRAMPercentage=N where 0 <= N <= 100.0 This option specifies maximum heap size. If -Xmx is specified, this option is ignored. 2. -XX:InitialRAMPercentage=N where 0 <= N <= 100.0 This option specifies initial heap size. If -Xms is specified, this option is ignored. Closes: eclipse-openj9#1428 Signed-off-by: Ashutosh Mehra <asmehra1@in.ibm.com>

ashu-mehra · 2018-05-23T11:08:50Z

@DanHeidinga Updated the changes as suggested.

DanHeidinga

lgtm

DanHeidinga · 2018-05-23T13:03:54Z

Jenkins test sanity plinux jdk8

DanHeidinga · 2018-05-23T13:04:46Z

Jenkins test sanity xlinux jdk8

DanHeidinga · 2018-05-23T13:05:14Z

Looks like the CI server is having issues. Once we can build this it can be merged

DanHeidinga · 2018-05-23T20:25:06Z

jenkins test sanity xlinux jdk8

pshipton added comp:vm comp:gc doc:whatsnew labels Apr 16, 2018

ashu-mehra force-pushed the add_gc_options_v3 branch from 3e56d86 to f921a5c Compare April 16, 2018 17:41

dmitripivkine requested review from dmitripivkine and amicic April 16, 2018 17:55

dmitripivkine self-assigned this Apr 16, 2018

dmitripivkine requested review from DanHeidinga and removed request for amicic April 16, 2018 18:33

dmitripivkine reviewed Apr 16, 2018

View reviewed changes

dmitripivkine requested review from dmitripivkine and amicic April 16, 2018 18:39

ashu-mehra force-pushed the add_gc_options_v3 branch 5 times, most recently from 4945ac1 to 127ca3c Compare April 24, 2018 11:20

ashu-mehra changed the title ~~WIP Specify heap limits as percentage of total memory~~ Specify heap limits as percentage of total memory Apr 24, 2018

dmitripivkine reviewed Apr 24, 2018

View reviewed changes

ashu-mehra mentioned this pull request Apr 26, 2018

Store addressable physical memory in GC_ExtensionsBase eclipse-omr/omr#2506

Merged

ashu-mehra force-pushed the add_gc_options_v3 branch 3 times, most recently from 4bb3066 to 031b795 Compare April 27, 2018 18:14

dmitripivkine reviewed May 8, 2018

View reviewed changes

ashu-mehra force-pushed the add_gc_options_v3 branch 2 times, most recently from 5f992ca to 0ea5491 Compare May 10, 2018 17:52

dmitripivkine reviewed May 14, 2018

View reviewed changes

ashu-mehra force-pushed the add_gc_options_v3 branch from 0ea5491 to 2489fc8 Compare May 14, 2018 17:26

dmitripivkine reviewed May 14, 2018

View reviewed changes

dmitripivkine approved these changes May 14, 2018

View reviewed changes

dmitripivkine removed the request for review from amicic May 14, 2018 18:34

SueChaplain mentioned this pull request May 17, 2018

Add support for specifying heap limit in terms of percentage of total memory #1428

Closed

DanHeidinga reviewed May 22, 2018

View reviewed changes

ashu-mehra force-pushed the add_gc_options_v3 branch from 2489fc8 to b46eb47 Compare May 23, 2018 08:12

ashu-mehra force-pushed the add_gc_options_v3 branch from b46eb47 to a945467 Compare May 23, 2018 08:40

DanHeidinga approved these changes May 23, 2018

View reviewed changes

pshipton added doc:externals and removed doc:whatsnew labels May 24, 2018

dmitripivkine merged commit 7aadebb into eclipse-openj9:master May 24, 2018

SueChaplain mentioned this pull request May 25, 2018

Setting the maximum heap size as a percentage of available memory eclipse-openj9/openj9-docs#25

Closed

ashu-mehra deleted the add_gc_options_v3 branch December 9, 2019 00:18

Specify heap limits as percentage of total memory #1699

Specify heap limits as percentage of total memory #1699

Conversation

ashu-mehra commented Apr 16, 2018 • edited Loading

pshipton commented Apr 16, 2018

ashu-mehra commented Apr 16, 2018

dmitripivkine commented Apr 16, 2018 • edited Loading

dmitripivkine Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashu-mehra commented Apr 24, 2018

ashu-mehra commented Apr 24, 2018

Choose a reason for hiding this comment

ashu-mehra Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

dmitripivkine Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

dmitripivkine May 8, 2018 • edited Loading

Choose a reason for hiding this comment

ashu-mehra May 10, 2018 • edited Loading

Choose a reason for hiding this comment

dmitripivkine May 10, 2018 • edited Loading

Choose a reason for hiding this comment

dmitripivkine May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine May 8, 2018 • edited Loading

Choose a reason for hiding this comment

ashu-mehra commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine left a comment

Choose a reason for hiding this comment

dmitripivkine commented May 14, 2018

dmitripivkine commented May 14, 2018

dmitripivkine commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashu-mehra May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashu-mehra commented May 23, 2018

DanHeidinga left a comment

Choose a reason for hiding this comment

DanHeidinga commented May 23, 2018

DanHeidinga commented May 23, 2018

DanHeidinga commented May 23, 2018

DanHeidinga commented May 23, 2018

ashu-mehra commented Apr 16, 2018 •

edited

Loading

dmitripivkine commented Apr 16, 2018 •

edited

Loading

dmitripivkine Apr 16, 2018 •

edited

Loading

ashu-mehra Apr 24, 2018 •

edited

Loading

dmitripivkine Apr 24, 2018 •

edited

Loading

dmitripivkine Apr 24, 2018 •

edited

Loading

dmitripivkine May 8, 2018 •

edited

Loading

ashu-mehra May 10, 2018 •

edited

Loading

dmitripivkine May 10, 2018 •

edited

Loading

dmitripivkine May 8, 2018 •

edited

Loading

dmitripivkine May 8, 2018 •

edited

Loading

ashu-mehra May 23, 2018 •

edited

Loading