Fix codecache Xlp parsing to use right-most option

[AlenBadel]

Currently, -Xlp:codecache:pagesize= takes priority no matter what position it is in the passed arguments.
The expected behaviour is that only the right-most instance of -Xlp be parsed, and that is exactly what this commit implements.

Signed-off-by: AlenBadel Alen.Badel@ibm.com

[AlenBadel]

A good example of the expected behaviour can be displayed by running

java -Xlp:codecache:pagesize=16M -Xlp64K
The current codecache will end up using 16M large pages, even when -Xlp64K is the right most option.

The expected behaviour is that 64K large pages be used. The objectheap has the correct behaviour. The following will cause the objectheap to use 64K large pages.
java -Xlp:objectheap:pagesize=16M -Xlp64K

See discussion: #4930

[AlenBadel]

FYI @gita-omr @pshipton

[gita-omr]

I wonder if we have any test cases to test this...

[pshipton]

There are tests for -Xlp
https://github.com/eclipse/openj9/blob/master/test/functional/VM_Test/src/j9vm/test/xlp/XlpOptionsTestRunner.java
and tests for -Xlp:codecache, just not for combining -Xlp:codecache and -Xlp options. It should be easy enough to add some to this suite. I see the -Xlp tests have cases with multiple options.
https://github.com/eclipse/openj9/blob/master/test/functional/VM_Test/src/j9vm/test/xlpcodecache/XlpCodeCacheOptionsTestRunner.java

[pshipton]

jenkins test sanity win jdk8

[pshipton]

jenkins test sanity aix,plinux,zlinux jdk8

[pshipton]

@smlambert @llxia I can't find any evidence that the xlp tests are running, can you please check.

I found this in the J9vmTest output:

  [j9vm.test.xlp] expiry: none :: This test is run separately and not as part of j9vm test suite
  [j9vm.test.xlpcodecache] expiry: none :: This test is run separately and not as part of j9vm test suite

[pshipton]

They are extended tests which are running internally but I don't find them at OpenJ9.

[pshipton]

Please squash.

[gita-omr]

There are tests for -Xlp
https://github.com/eclipse/openj9/blob/master/test/functional/VM_Test/src/j9vm/test/xlp/XlpOptionsTestRunner.java
and tests for -Xlp:codecache, just not for combining -Xlp:codecache and -Xlp options. It should be easy enough to add some to this suite. I see the -Xlp tests have cases with multiple options.
https://github.com/eclipse/openj9/blob/master/test/functional/VM_Test/src/j9vm/test/xlpcodecache/XlpCodeCacheOptionsTestRunner.java

@AlenBadel let's add the missing combinations.

[pshipton]

fyi the copyright check failed, and there is a merge conflict.

[AlenBadel]

@AlenBadel let's add the missing combinations.

This test seems to just verify the objectheap page size.

 * This test checks different flavors of -Xlp option except -Xlp:codecache
 * It uses -Xlp options along with -verbose:gc to verify the working of -Xlp options.

This test would need to be modified to pull the codecache sizes.

I agree that we need to add the codecache option variations to this, but we also need to add tests for the two proposed hotspot mappings -XX:+UseLargePages and XX:LargePageSizeInBytes=<size> proposed in #4930.

I propose that I add it as a to-do item in the main issue above, and we make this test change to add tests for codecache, and the two mapped options together when the final changes are in.

[gita-omr]

Sure, let's open a separate item for the new test cases.

[AlenBadel]

Sure, let's open a separate item for the new test cases.

See #4930 (comment)
I've commented inside the parent issue as a requirement.

[AlenBadel]

@dmitripivkine Could you kindly review the comments I left on your comment, and resolve it if you don't have any further concerns?

[dmitripivkine]

I still have concerns about form of the change (mix functional and comment changes, comments format change is unnecessary). This is up to code owners keep change reasonable. However I have not reviewed this change functionally. I believe this change should be approved by code owner (JIT) reviewer

[AlenBadel]

@gita-omr Could I request that you review this functionally?

[gita-omr]

@gita-omr Could I request that you review this functionally?

Sure, a little bit later today.