JDK-8322943: runtime/CompressedOops/CompressedClassPointers.java fails on AIX

[JoKern65]

Even after recent fixes like
https://bugs.openjdk.org/browse/JDK-8305765
the test runtime/CompressedOops/CompressedClassPointers.java fails on AIX.

This error results from the fact, that on AIX the shmat() allocation granularity is 256MB instead of the standard Pagesize (4KB or 64KB).

Because my first proposal (PR 17708) of introducing a new method os::vm_shm_allocation_granularity() in the shared hotspot code was rejected I now encapsulate the difference in ifdef AIX brackets.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8322943: runtime/CompressedOops/CompressedClassPointers.java fails on AIX (Bug - P4)

Reviewers

• Guoxiong Li (@lgxbslgx - Committer) ⚠️ Review applies to 0aa55745
• Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18105/head:pull/18105
$ git checkout pull/18105

Update a local copy of the PR:
$ git checkout pull/18105
$ git pull https://git.openjdk.org/jdk.git pull/18105/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18105

View PR using the GUI difftool:
$ git pr show -t 18105

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18105.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jkern! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@JoKern65 The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 03: Full - Incremental (33e2b056)
• 02: Full - Incremental (179b2474)
• 01: Full - Incremental (4bed19d1)
• 00: Full (0aa55745)

[lgxbslgx]

I don't have a AIX device to run the related test. But the patch looks good.

A trivially possible problem is that it seems not so good to repeat the same comments in all the code places. But I don't have a better alternative way to solve such issue. So I am OK with it now.

[tstuefe]

I don't think we need to cater to the 4K page mode. It was originally added for AIX versions that don't support 64K pages. AIX 4.xx IIRC. The need for it should be long gone.

I am not doing AIX coding anymore, but my advice would be to simplify and require 64k pages always. WRT this change, the 4K page handling is certainly not needed. Just hardcode to SHMLBA. Or 256M, whatever causes the least amount of ifdefs.

Other than that, please slim the fix down to the necessary. The many copypasted sections are really not pretty.

The only really needed parts are the one in os::reserve_memory_inbetween. This is a small issue, let's keep the patch small and confined.

[tstuefe]

Unnecessary for the fix. And the old code does basically the same, only less verbose.

[GoeLin]

Hi @tstuefe, this is a fix for an existing issue. Changing the behavior of the AIX allocation is not the intention of this fix. Modernizing the AIX memory allocation is a possible follow-up. Also we need to backport this, so it should be fix-only. Thus I think we should really do the 4K/256M distiction here to match the allocation implemented in os_aix.cpp.

[tstuefe]

Hi @GoeLin ,

not sure what you understood from my comment, but it was not what I was writing.

Hi @tstuefe, this is a fix for an existing issue. Changing the behavior of the AIX allocation is not the intention of this fix. Modernizing the AIX memory allocation is a possible follow-up.

Which I did not propose? Quoting myself here:

"Other than that, please slim the fix down to the necessary...
The only really needed parts are the one in os::reserve_memory_inbetween. This is a small issue, let's keep the patch small and confined."

Also we need to backport this, so it should be fix-only.

Backport to where? os::reserve_memory_inbetween was introduced with https://bugs.openjdk.org/browse/JDK-8312018, which was introduced with JDK 22. The hunk affecting virtualspace.cpp has nothing to do with this problem, and should not be part of this patch.

Thus I think we should really do the 4K/256M distiction here to match the allocation implemented in os_aix.cpp.

Which does not follow from "it should be a minimal fix". The 4K scenario is fictional. And even if it were not, aligning reservations to SHMLBA is never wrong, not even if you use mmap. So why the added complexity?

To be clear, I dislike it because of the the n times copy-pasted logic spread over the code base. If it were just one hunk in os.cpp, I would not care about the 4K page mode.

[GoeLin]

Hi @tstuefe, I read and understood all of your reply. I was referring to your "I don't think we need to cater to the 4K page mode." statement. The 4K page mode is current state of the AIX implementation, and in my eyes the error is in virtualspace and os.cpp to use a wrong alignement for the attach points, not in os_aix.cpp.
Also, for the attach point randomization, it well makes a difference if you use 4K or 256M. There are less than 16 attach points aligned to 256M for a 1G allocation request within 4G, which makes allocation in the lowest 4G fail (unscaled mode)
.
I was not aware the os.cpp problem appeared only after 21.

For the n-times copied logic: There are only two places in the JVM coding. The others are in tests. Obviously, the tests must be adapted to the JVM coding.
Joachim's original proposal was to encapsulate this in a new function. This moves the logic into the aix coding. You voted against this. So what to do now?

[JoKern65]

With my new commit I tried to be less verbose.
And indeed os::attempt_reserve_memory_between() is not in jdk21, but introduced by you, Thomas some months ago.

[tstuefe]

Okay. Thanks for reducing the patch size.

[openjdk]

@JoKern65 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8322943: runtime/CompressedOops/CompressedClassPointers.java fails on AIX

Reviewed-by: gli, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 64 new commits pushed to the master branch:

• bdd1aeb: 8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection
• fb4610e: 8327444: simplify RESTARTABLE macro usage in JDK codebase
• de428da: 8327426: RISC-V: Move alignment shim into initialize_header() in C1_MacroAssembler::allocate_array
• 8dbd4b3: 8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate
• 7c5e6e7: 8327147: Improve performance of Math ceil, floor, and rint for x86
• 972e81d: 8326611: Clean up vmTestbase/nsk/stress/stack tests
• 5aae803: 8327390: JitTester: Implement temporary folder functionality
• 784f11c: 8327238: Remove MetadataAllocationFailALot* develop flags
• d7273ac: 8320646: RISC-V: C2 VectorCastHF2F
• 53c4714: 8327501: Common ForkJoinPool prevents class unloading in some cases
• ... and 54 more: https://git.openjdk.org/jdk/compare/31ac8714e0593f2feaa8e9ebaf32bab904ba6d11...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@lgxbslgx, @tstuefe) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[JoKern65]

/integrate

[openjdk]

@JoKern65
Your change (at version 33e2b05) is now ready to be sponsored by a Committer.

[MBaesken]

/sponsor

[openjdk]

Going to push as commit 997e615.
Since your change was applied there have been 68 commits pushed to the master branch:

• 27a03e0: 8327218: Add an ability to specify modules which should have native access enabled
• d0d4912: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater
• c65da92: 8327571: Parallel: Remove redundant operation in PSParallelCompact::clear_data_covering_space
• f9d479f: 8327477: Parallel: Remove _data_location and _highest_ref in ParallelCompactData
• bdd1aeb: 8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection
• fb4610e: 8327444: simplify RESTARTABLE macro usage in JDK codebase
• de428da: 8327426: RISC-V: Move alignment shim into initialize_header() in C1_MacroAssembler::allocate_array
• 8dbd4b3: 8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate
• 7c5e6e7: 8327147: Improve performance of Math ceil, floor, and rint for x86
• 972e81d: 8326611: Clean up vmTestbase/nsk/stress/stack tests
• ... and 58 more: https://git.openjdk.org/jdk/compare/31ac8714e0593f2feaa8e9ebaf32bab904ba6d11...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken @JoKern65 Pushed as commit 997e615.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.