Parse Large Code Cache Page Sizes Options Correctly #22190
Conversation
luke-li-2003
force-pushed
the
CodeCachePageOptFix
branch
from
99e30a6 to
c0e1e30
Compare
|
Issue: #22143 |
|
FYI @zl-wang |
|
If this commit fixes the problem, can you also include a commit in this PR that re-enables the tests on AIX? They were disabled here -> #22189 |
luke-li-2003
force-pushed
the
CodeCachePageOptFix
branch
from
c0e1e30 to
2b3d335
Compare
|
I wonder why we didn't see a problem when testing the modified tests last week (both externally and internally)? That gives me pause (albeit small) to suggest enabling the tests right away since we don't know why they passed last week. |
|
I had missed it because it's in extended.functional rather than sanity, and it's AIX-exclusive since XlpCodeCacheOptionsTestRunner.java only tests for 16G on AIX. Is there an example of it passing on AIX last week? |
| @@ -2135,7 +2135,7 @@ bool J9::Options::preProcessCodeCacheXlpCodeCache(J9JavaVM *vm, J9JITConfig *jit | |||
|
|
|||
| if (largePageSize > (0) && isNonNegativePowerOf2((int32_t)largePageSize)) | |||
There was a problem hiding this comment.
There shouldn't be a cast here either if largePageSize can be > 2GB
luke-li-2003
force-pushed
the
CodeCachePageOptFix
branch
from
2b3d335 to
02522aa
Compare
Thanks for clearing that up.
No, I only ran the builds that the OMR Acceptance would run (sanity.functional, sanity.openjdk). |
|
Updated code, hope that works. |
|
It was passing in builds before OMR promoted. |
You can run a grinder to test it. Do you need help to set that up? |
|
I presume before promotion meant it was still using the downstream code without any of my code cache changes? If that's the case it is expected to pass, since a 16G page size should just be rejected. |
Right. |
|
Also the link above seems to be a mac build, I grabbed an aix build that hopefully is representative (https://openj9-jenkins.osuosl.org/job/Test_openjdk11_j9_extended.functional_ppc64_aix_Nightly_testList_1/1002/consoleText), and its behaviour is matching what I expect before my changes: There shouldn't be a case where the test still somehow passes for AIX after my changes were promoted. |
Right, it failed everywhere it ran. The grinder I started to test this change passed on jdk11. |
Remove an inappropriate cast that breaks the -Xlp:codecache:pagesize=<> option larger than 32-bit (e.g. 16G page size on AIX). Also re-enable the xlpCodeCacheTests test case for AIX Issue: eclipse-openj9#22143 Signed-off-by: Luke Li <[email protected]>
luke-li-2003
force-pushed
the
CodeCachePageOptFix
branch
from
02522aa to
f183f39
Compare
|
Jenkins test sanity all jdk21 |
|
All looks good in testing and Peter's grinder. @pshipton, do you approve? |
|
lgtm |
| if (largePageSize > (0) && isNonNegativePowerOf2((int32_t)largePageSize)) | ||
| if (largePageSize > (0) | ||
| #if defined(J9VM_ENV_DATA64) | ||
| && isNonNegativePowerOf2((int64_t) largePageSize) |
There was a problem hiding this comment.
The parameter of isNonNegativePowerOf2() is int32_t so the cast change here doesn't change that part of the test.
isNonNegativePowerOf2() is broken: isNonNegativePowerOf2(16G) becomes isNonNegativePowerOf2(0) (after narrowing to 32 bits) which should answer false, but does not. I'll create an OMR change to fix that.
Edit: I now see that isNonNegativePowerOf2() is overloaded: there is a 64-bit version as well, but they both give the wrong answer for zero.
Remove an inappropriate cast that breaks the -Xlp:codecache:pagesize=<> option larger than 32-bit (e.g. 16G page size on AIX).