Add Support for Hotspot Codecache and Objectheap Options #7476

AlenBadel · 2019-10-16T15:21:04Z

This commit adds functionality to parse and set the respective large page sizes within the codecache and objectheap when passed by the user using the options -XX:LargePageSizeInBytes=<size> and -XX:+UseLargePages.

-XX:+UseLargePages
Both the objectheap and codecache will use the largest available page size on the system.

-XX:LargePageSizeInBytes=<size>
Both the objectheap and codecache will use attempt to use the page size specified by the user. If that is not available, then a default value is used.

Signed-off-by: AlenBadel [email protected]

AlenBadel · 2019-10-16T15:23:47Z

See Issue #4930

Parsing

I've added the main parsing for -XX:+UseLargePages and -XX:LargePageSizeInBytes=<size> within jvminit.c.
I've added uintptr_t largePageSizeRequested, and IDATA largePageArgIndex inside J9JavaVM.
When -XX:+UseLargePages is the right most option, largePageArgIndex will contain it's index, and largePageSizeRequested will be set to -1.
When -XX:LargePageSizeInBytes=<size>is the right most option, largePageArgIndex will contain it's index, and largePageSizeRequested will contain the parsed size parameter.
When both -XX:LargePageSizeInBytes=<size> and -XX:+UseLargePages are used the right most option overwrites any other parameter that controls the large page size including Xlp<sizse>, Xlp, Xlp:codecache:pagesize=<size>, and Xlp:objectheap:pagesize=<size>.

Codecache

I've simply just added for the handling for the two options as we handle the other -Xlp options discussed above inside J9Options.cpp

Objectheap

I've created a routine called gcParseXXObjectHeapArguments which sets up the objectheap large page size if the argument index of -XX:+UseLargePages and -XX:LargePageSizeInBytes=<size> is on the right of all other Xlp options.
To verify the right most Xlp option, I've changed the return type of gcParseXlpOption to an Integer rather than a boolean. That method will return the index of the right most index of Xlp if one is used, 0 if not used and Xlp parsing passed, and -1 if there has been any error in parsing.
I've went ahead and moved the functionality of verifying the parsed argument as used in -Xlp<size> and -Xlp:objecheap:pagesize=<size> into a general method verifyLargePageSize
I've also moved the functionality of setting the large page size to be the largest available on the system as used when passing -Xlp into a general routine. This method is used when parsing

pshipton · 2019-10-16T16:29:59Z

@dmitripivkine

DanHeidinga

Please have a conversation with @dmitripivkine about how he'd like to see the mappings handled for the GC.

The COPY_OPTION_OPTION adds a lot of complexity that doesn't seem to pay off. I would rather see a single place that processes the option and have all consumers share the single parsed view of the data

runtime/compiler/control/J9Options.cpp

runtime/oti/jvminit.h

runtime/util/vmargs.c

runtime/vm/jvminit.c

runtime/gc_modron_startup/mmparse.cpp

pshipton · 2019-10-16T17:11:02Z

This requires an Issue at https://github.com/eclipse/openj9-docs before merge.

runtime/gc_modron_startup/mmparse.cpp

dmitripivkine · 2019-10-17T16:37:51Z

@joransiu
We are trying to get solution how to match -XX:LargePageSizeInBytes= on ZOS
Currently for -Xlp there is mandatory (for ZOS) suboption pageable/nonpageable
Typical ZOS pages choice is:

4k nonpageable
1m pageable <--- default
1m nonpageable
2g nonpageable
What if we base at page size (if there is only one kind) or use pageable(?) by default if both types with requested size available?
for example:
-XX:LargePageSizeInBytes=2147483648 means 2g nonpageable (only one kind of 2g pages available)
-XX:LargePageSizeInBytes=1048576 means 1m pageable (both kinds available but pageable used by default)
Would it be sufficient?

joransiu · 2019-10-17T16:57:05Z

-XX:LargePageSizeInBytes=2147483648 means 2g nonpageable (only one kind of 2g pages available)
-XX:LargePageSizeInBytes=1048576 means 1m pageable (both kinds available but pageable used by default)
Would it be sufficient?

Yeah, that sounds reasonable to me. We would generally prefer 1MB pageable over non-pageable if both are available.

AlenBadel · 2019-10-17T18:38:12Z

@joransiu Thanks for confirming. I'll make that change.

pshipton · 2019-10-23T17:54:26Z

For the documentation requirements, please take a look at eclipse-openj9/openj9-docs#408

dmitripivkine · 2020-03-09T15:32:53Z

runtime/oti/j9nonbuilder.h

+	 * largePageArgIndex - The right most index of either option.
+	 */
+	IDATA largePageSizeRequested;
+	IDATA largePageArgIndex;


I am not sure that J9VM is a best place to pass parameters but I am ok if VM developers can tolerate this

@pshipton Could you confirm this?

I'm not a huge fan of this approach but can live with it if the two fields are wrapped in an inline struct.

Please introduce a new struct something like the following to hold the two values:

typedef struct J9LargePageCompatibiltyOptions { IDATA pageSizeRequested; IDATA optionIndex; }

dmitripivkine · 2020-03-09T15:45:58Z

runtime/oti/jvminit.h

@@ -512,6 +512,8 @@ enum INIT_STAGE {
 #define MAPOPT_XXONOUTOFMEMORYERROR_EQUALS "-XX:OnOutOfMemoryError="
 #define MAPOPT_XXENABLEEXITONOUTOFMEMORYERROR "-XX:+ExitOnOutOfMemoryError"
 #define MAPOPT_XXDISABLEEXITONOUTOFMEMORYERROR "-XX:-ExitOnOutOfMemoryError"
+#define MAPOPT_XXLARGEPAGESIZEINBYTES_EQUALS "-XX:LargePageSizeInBytes="
+#define MAPOPT_XXUSELARGEPAGES "-XX:+UseLargePages"


Should it be recognition of "-XX:-UseLargePages" for symmetry ? I am neutral about this. Asking for VM Reviewer opinion

@pshipton Should this be added?

In theory, it makes sense to have an option to disable all large page options, including -Xlp*. However this PR is to map Hotspot Options, and Hotspot does not support such an option.

Yes, this needs to have the -XX:+UseLargePages to be consistent with our standard processes. If there is a + version, there must be a - version as well. Otherwise, use the non-[+-] -XX:Foo style option

runtime/vm/jvminit.c

dmitripivkine · 2020-03-09T16:03:38Z

I did review for GC part of changes only. With my comments addressed it looks correct to me.

runtime/compiler/control/J9Options.cpp

dmitripivkine · 2020-03-09T16:58:46Z

runtime/gc_modron_startup/mmparse.cpp

 gcParseXlpOption(J9JavaVM *vm)
 {
 	MM_GCExtensions *extensions = MM_GCExtensions::getExtensions(vm);
-	bool rc = false;
+	IDATA rc = -2;


would you please add comment before this line to explain in simple words what this return code means

That's the intention. Could you address my comments here? #7476 (comment). If you're ok with that change I can go ahead with commenting the codes.

dmitripivkine · 2020-03-09T17:01:08Z

runtime/gc_modron_startup/mmparse.cpp

 		goto _error;
 	}

+	/* Setup to handle additional objectheap arguments */
+	if ((vm->largePageArgIndex != -1) && (vm->largePageArgIndex > xlpOptionIndex) && (!gcProcessXXLargePageObjectHeapArguments(vm))) {


please put constant first (-1 != vm->largePageArgIndex).

With xlpOptionIndex can be -2 now the (vm->largePageArgIndex > xlpOptionIndex) condition looks confusing for case vm->largePageArgIndex is -1 -1 > -2. It is works just because it is guarded by (vm->largePageArgIndex != -1) as first condition. So it is correct code flow wise but has underwater mine for somebody might need change this code in the future

As long as the return codes are correctly commented, then it shouldn't be an issue for anyone looking at in the future.

The principle Dmitri is aiming for relates to "code is read more than it's written" and "we have limited brain space, don't spend it on the trivial".

By making code obviously correct, we don't have worry or double check this when debugging though this path in the future.

In this case I can suggest a better solution.
gcParseXlpOption What we can do is simply revert the initial idea of returning the index and have the method return the boolean. The return value will return false if there has been an error in consuming the options.

We can then add an argument to the method by reference, where it can be populated to either the index of the right-most consumed index if one exists, or -1 if no argument was consumed.

That would very much simplify the comparison after calling that method.

DanHeidinga · 2020-03-09T17:10:35Z

runtime/nls/j9vm/j9vm.nls

@@ -1654,6 +1654,20 @@ J9NLS_VM_IDLE_TUNING_PLATFORM_OR_GC_POLICY_NOT_SUPPORTED.system_action=The JVM w
 J9NLS_VM_IDLE_TUNING_PLATFORM_OR_GC_POLICY_NOT_SUPPORTED.user_response=Refer to the documentation for supported platforms and GC policies.
 # END NON-TRANSLATABLE

+J9NLS_VM_OPTIONS_MUST_BE_NUMBER=%s must be followed by a number


Please review the guidance on adding NLS messages at the top of every NLS file:

# Note to developers: # # New messages MUST be added at the end of this file. # DO NOT delete messages from this file, as it will change their indices. # If you wish to remove a message, delete its text, but leave the key in place

DanHeidinga · 2020-03-09T17:15:34Z

runtime/oti/jvminit.h

@@ -512,6 +512,8 @@ enum INIT_STAGE {
 #define MAPOPT_XXONOUTOFMEMORYERROR_EQUALS "-XX:OnOutOfMemoryError="
 #define MAPOPT_XXENABLEEXITONOUTOFMEMORYERROR "-XX:+ExitOnOutOfMemoryError"
 #define MAPOPT_XXDISABLEEXITONOUTOFMEMORYERROR "-XX:-ExitOnOutOfMemoryError"
+#define MAPOPT_XXLARGEPAGESIZEINBYTES_EQUALS "-XX:LargePageSizeInBytes="
+#define MAPOPT_XXUSELARGEPAGES "-XX:+UseLargePages"


Yes, this needs to have the -XX:+UseLargePages to be consistent with our standard processes. If there is a + version, there must be a - version as well. Otherwise, use the non-[+-] -XX:Foo style option

DanHeidinga · 2020-03-09T17:27:27Z

runtime/vm/jvminit.c

+				IDATA argIndexUseLargePages = FIND_AND_CONSUME_ARG(EXACT_MATCH, MAPOPT_XXUSELARGEPAGES, NULL);
+				IDATA argIndexLargePageSizeInBytes = FIND_AND_CONSUME_ARG(STARTSWITH_MATCH, MAPOPT_XXLARGEPAGESIZEINBYTES_EQUALS, NULL);
+
+				if (argIndexUseLargePages > argIndexLargePageSizeInBytes) {


Does this match the RI's behaviour? Do they treat -XX:LargePageSizeInBytes=1m -XX:+UseLargePages differently than -XX:+UseLargePages -XX:LargePageSizeInBytes=1m? Is there verbose output of some sort that shows that?

Is there verbose output of some sort that shows that?

Unfortunately there isn't. We only know that both these options affect both the codecache and objectheap. The design was a piggy-back on how -Xlp determines which option to use; the rightmost always wins.

DanHeidinga · 2020-03-09T17:53:29Z

runtime/vm/jvminit.c

+			/* Parse options related to Large Pages */
+			{
+				IDATA argIndexUseLargePages = FIND_AND_CONSUME_ARG(EXACT_MATCH, MAPOPT_XXUSELARGEPAGES, NULL);
+				IDATA argIndexLargePageSizeInBytes = FIND_AND_CONSUME_ARG(STARTSWITH_MATCH, MAPOPT_XXLARGEPAGESIZEINBYTES_EQUALS, NULL);


The approach taken here is to decide which of the two new arguments has priority and only record it. It leaves the priority of this option versus the old -Xlp-style options to be re-implemented by the consumers.

I would prefer to have that decision centralized here rather than spread out and potentially handled inconsistently in the different components due to future changes.

We have an some macros that make it easy to find an option and then to iteratively search forward by the different matches. See an example in the verifier option handling: [1]

The idea would be to do something similar and find the first -Xlp* option here using a STARTSWITH_MATCH and then marking each -Xlp option that occurs before the new options as "consumed" using the CONSUME_ARG(j9vm_args, element) macro.

JIT and GC processing then would only have 1 set of data to examine - either the new options data or the old Xlp style data and every component would have a consistent view of the data.

[1] https://github.com/eclipse/openj9/blob/d6506efbb6f74e7826f7c825c76489ab6e8ba944/runtime/bcverify/bcverify.c#L2694-L2713

This is certainly a great idea. There's just one caveat.

Since these -XX options impact both the codecache and objectheap then -Xlp options can be ignored if they precede these options.
The opposite is not true, since -Xlp, -Xlp:codecache, and -Xlp:objectheap selectively impact either the objectheap, or codecache only. It is assumed that the respective components handle these partial options.

The changes I've made allow for the codecache and objectheap to handle any combination of -Xlp*, XX:+UseLargePages, and -XX:LargePageInBytes=<size>.
So this could still work, given that if a combination of -Xlp, and -XX options are passed and if the right most option is -Xlp then we can't simply consume the -XX page options unless we start keeping track of for which -Xlp option impacts the codecache (-Xlp -Xlp:codecache), and which impact the objectheap(-Xlp, -Xlp, -Xlp:objectheap).

An example of this is using the following:
-XX:+UseLargePages -Xlp:objectheap:pagesize=2G
Codecache would use 1M pages, while objectheap would use 2G pages.

Should we disallow the user from using a combination of -XX page options and -Xlp options entirely?

I suspect the best approach is to keep track of options that impact the objectheap, and codecache separately. Then consume all options that don't apply. Meaning the worst case scenario would be that two options would not be consumed. One option will be elusively applied to the codecache, and the other to the objectheap.

Code for my suggestion: #9757

@DanHeidinga

Since these -XX options impact both the codecache and objectheap then -Xlp options can be ignored if they precede these options.
The opposite is not true, since -Xlp, -Xlp:codecache, and -Xlp:objectheap selectively impact either the objectheap, or codecache only. It is assumed that the respective components handle these partial options.

As I've mentioned. It's not possible to isolate between either using -Xlp:* options or the -XX options unless we make a decision to ignore all -Xlp:* options when -XX options are present no matter of the index within vmargs.

The closest thing we can do is flag to ignore options that will not be used within the codecache or the objectheap. In this case all -Xlp options will be ignored, if any of the -XX options are used at a later index.
Examples
-Xlp -XX:+UseLargePages. (-Xlp is ignored)
-Xlp:+UseLargePages -Xlp:codecache:pagesize= (No option is ignored. UseLargePages will be used by the objectheap, and -Xlp:codecache used by the codecache).

DanHeidinga · 2020-03-09T17:57:24Z

runtime/oti/j9nonbuilder.h

+	 * largePageArgIndex - The right most index of either option.
+	 */
+	IDATA largePageSizeRequested;
+	IDATA largePageArgIndex;


I'm not a huge fan of this approach but can live with it if the two fields are wrapped in an inline struct.

Please introduce a new struct something like the following to hold the two values:

typedef struct J9LargePageCompatibiltyOptions { IDATA pageSizeRequested; IDATA optionIndex; }

AlenBadel · 2020-03-18T15:06:56Z

@DanHeidinga I appreciate your feedback. I've went ahead and implemented some of your suggestions and I'm awaiting your response on a few others.

DanHeidinga · 2020-03-18T17:05:38Z

Sorry @AlenBadel It's going to take me a bit to circle back to this as I'm currently working my way through my 0.20.0 reviews.

AlenBadel · 2020-06-01T20:01:07Z

I think one of more fundamental questions we need to address is if the proposed -XX options should override -Xlp:* options, no matter what index they are within the vmargs. In the design it was assumed they would follow the "right-most" precedence rule; allowing both -Xlp and -XX options to be used together.

A question likely best answered by @pshipton & @DanHeidinga

DanHeidinga · 2020-06-02T15:04:02Z

I think one of more fundamental questions we need to address is if the proposed -XX options should override -Xlp:* options, no matter what index they are within the vmargs. In the design it was assumed they would follow the "right-most" precedence rule; allowing both -Xlp and -XX options to be used together.

As you point out in #8671 (comment), parsing of the existing options is already somewhat inconsistent in how -Xlp vs -Xlp:... is handled.

The current -Xlp:... options would look very different if they were being developed today. Each sub-option would be its own -XX:[+-] form. So -Xlp:objectheap:pagesize=2G would be written as -XX:+UseLargePagesObjectHeap -XX:LargePageSizeInBytes=2G. And if we need to be able to pick different different large page sizes for the code cache and heap we may add: -XX:CodeCacheLargePageSizeInBytes=... & -XX:ObjectHeapLargePageSizeInBytes=.... Though if we don't need the flexibility we should get rid of it.

That's the model we should be working toward.

With that in mind, the direction I'd like to take is to:

Deprecate the existing -Xlp & -Xlp:... options in the documentation
Add new -XX:.... forms for the required sub-options as described above. We need a design that maps the sub-options to their new -XX:[+-] forms.
Parse the new -XX large page options.
- Note, that sizes will only take affect if LargePages is enabled. On some platforms, LargePages may be default enabled.
During parsing, map the legacy -Xlp:... options to their new -XX:[+-] forms and only operate on the new -XX:[+-] forms.
Eventually remove support fo the -Xlp: formats

@pshipton @dmitripivkine do you have any concerns with this direction?

pshipton · 2020-06-02T15:35:41Z

+1

dmitripivkine · 2020-06-02T15:53:22Z

I do support proposed direction. I hope it will help us with complications with default large page size we have historically on AIX and ZOS

gita-omr · 2020-06-02T16:11:14Z

The behaviour of the -Xlp... options will obviously change. Are we OK with that? Previously we put a lot of effort to avoid that. We had a design that was approved. This is an absolutely new design and we need to start all over again.

DanHeidinga · 2020-06-03T01:54:46Z

The behaviour of the -Xlp... options will obviously change. Are we OK with that?

@gita-omr which cases will change? If there are examples that would change meaning under this design, let's look at them in detail and see what the effects would be.

gita-omr · 2020-06-03T02:20:38Z

The behaviour of the -Xlp... options will obviously change. Are we OK with that?

@gita-omr which cases will change? If there are examples that would change meaning under this design, let's look at them in detail and see what the effects would be.

Hi @DanHeidinga . Yes, we are preparing a detailed summary of all the large page options, which will make it easier to see all the subtle differences and interactions among them. Hopefully that document will help make all the decisions and will become a final specification.

This commit adds functionality to parse and set the respective large page sizes within the codecache and objectheap when passed by the user using the options -XX:LargePageSizeInBytes=<size> and -XX:+UseLargePages. -XX:+UseLargePages Both the objectheap and codecache will use the largest available page size on the system. -XX:LargePageSizeInBytes=<size> Both the objectheap and codecache will use attempt to use the page size specified by the user. If that is not available, then a default value is used. Signed-off-by: AlenBadel <[email protected]>

AlenBadel · 2020-07-28T21:28:39Z

@DanHeidinga Since the discussion, and inital design that this PR was created with has greatly diverged would you agree to close this PR in favour for a new PR that implements the design specified here #8671?

DanHeidinga · 2020-07-29T03:49:06Z

Agreed. Closing as this is obsoleted by #8671

AlenBadel changed the title ~~WIP: Mapping GC Options~~ Mapping GC Options Oct 16, 2019

dmitripivkine added comp:gc comp:jit comp:vm labels Oct 16, 2019

DanHeidinga requested a review from dmitripivkine October 16, 2019 16:30

DanHeidinga reviewed Oct 16, 2019

View reviewed changes

pshipton added the depends:doc label Oct 16, 2019

AlenBadel force-pushed the manyMapping branch from 5208a45 to f0c694e Compare October 16, 2019 17:24

dmitripivkine reviewed Oct 16, 2019

View reviewed changes

runtime/gc_modron_startup/mmparse.cpp Outdated Show resolved Hide resolved

AlenBadel mentioned this pull request Oct 17, 2019

Remove Unused Options eclipse-omr/omr#4478

Merged

AlenBadel force-pushed the manyMapping branch from a8b4b6e to 9c8ea71 Compare October 18, 2019 17:24

AlenBadel changed the title ~~Mapping GC Options~~ WIP :Mapping GC Options Oct 22, 2019

AlenBadel force-pushed the manyMapping branch 3 times, most recently from ab98355 to 3b069be Compare October 24, 2019 21:12

AlenBadel force-pushed the manyMapping branch 2 times, most recently from 631c59d to 50a9ae1 Compare November 7, 2019 16:27

AlenBadel force-pushed the manyMapping branch 4 times, most recently from 4d01bd3 to a71e173 Compare December 11, 2019 15:56

dmitripivkine self-requested a review December 11, 2019 15:59

AlenBadel force-pushed the manyMapping branch from 50deb8a to 5813792 Compare December 11, 2019 16:02

dmitripivkine reviewed Mar 9, 2020

View reviewed changes

runtime/vm/jvminit.c Outdated Show resolved Hide resolved

dmitripivkine reviewed Mar 9, 2020

View reviewed changes

runtime/compiler/control/J9Options.cpp Outdated Show resolved Hide resolved

dmitripivkine reviewed Mar 9, 2020

View reviewed changes

runtime/compiler/control/J9Options.cpp Show resolved Hide resolved

dmitripivkine reviewed Mar 9, 2020

View reviewed changes

DanHeidinga requested changes Mar 9, 2020

View reviewed changes

AlenBadel force-pushed the manyMapping branch 3 times, most recently from c52f66c to 491cde3 Compare March 10, 2020 13:43

AlenBadel closed this Mar 10, 2020

AlenBadel force-pushed the manyMapping branch from 491cde3 to a5a4b07 Compare March 10, 2020 13:47

AlenBadel reopened this Mar 10, 2020

AlenBadel mentioned this pull request Mar 18, 2020

WIP: Add new tests for new options #8820

Open

AlenBadel force-pushed the manyMapping branch from 491cde3 to 771dd33 Compare July 28, 2020 20:58

DanHeidinga closed this Jul 29, 2020

Add Support for Hotspot Codecache and Objectheap Options #7476

Add Support for Hotspot Codecache and Objectheap Options #7476

Conversation

AlenBadel commented Oct 16, 2019 • edited Loading

AlenBadel commented Oct 16, 2019 • edited Loading

pshipton commented Oct 16, 2019

DanHeidinga left a comment

Choose a reason for hiding this comment

pshipton commented Oct 16, 2019

dmitripivkine commented Oct 17, 2019

joransiu commented Oct 17, 2019

AlenBadel commented Oct 17, 2019

pshipton commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlenBadel Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlenBadel Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlenBadel Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

AlenBadel Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

AlenBadel Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlenBadel commented Mar 18, 2020

DanHeidinga commented Mar 18, 2020

AlenBadel commented Jun 1, 2020

DanHeidinga commented Jun 2, 2020

pshipton commented Jun 2, 2020

dmitripivkine commented Jun 2, 2020

gita-omr commented Jun 2, 2020

DanHeidinga commented Jun 3, 2020

gita-omr commented Jun 3, 2020

AlenBadel commented Jul 28, 2020

DanHeidinga commented Jul 29, 2020

AlenBadel commented Oct 16, 2019 •

edited

Loading

AlenBadel commented Oct 16, 2019 •

edited

Loading

dmitripivkine Mar 9, 2020 •

edited

Loading

AlenBadel Mar 9, 2020 •

edited

Loading

AlenBadel Mar 9, 2020 •

edited

Loading

AlenBadel Mar 11, 2020 •

edited

Loading

AlenBadel Mar 11, 2020 •

edited

Loading

AlenBadel Mar 11, 2020 •

edited

Loading