8345655: Move reservation code out of ReservedSpace

[stefank]

The ReservedSpace class and its friends has a dual functionality of describing a reserved memory region AND it also reserves memory. I would like to split this so that the ReservedSpace classes only acts as data carrier about reserved memory and then have a more explicit API for reserving memory and baking a ReservedSpace given the outcome of the reservation.

See the first comment for the full description:

---

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-8345655: Move reservation code out of ReservedSpace (Enhancement - P4)

Reviewers

• Afshin Zafari (@afshin-zafari - Committer)
• Johan Sjölen (@jdksjolen - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22712

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back stefank! 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]

@stefank 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:

8345655: Move reservation code out of ReservedSpace

Reviewed-by: azafari, jsjolen

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 40 new commits pushed to the master branch:

• 725079b: 8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException
• 5e25c48: 8346289: Confusing phrasing in IR Framework README / User-defined Regexes
• fbbc7c3: 8346120: VirtualThreadPinned event recorded for Object.wait may have wrong duration or may record second event
• 466c00a: 8346234: javax/swing/text/DefaultEditorKit/4278839/bug4278839.java still fails in CI
• bd3c0be: 8268611: jar --validate should check targeted classes in MR-JAR files
• 87804f2: 8346294: Invalid lint category specified in compiler.properties
• 18d1d61: 8346046: Enable copyright header format check
• a7631cc: 8346235: RISC-V: Optimize bitwise AND with mask values
• 929d4a5: 8346231: RISC-V: Fix incorrect assertion in SharedRuntime::generate_handler_blob
• 3030230: 8346278: Clean up some flag handing in flags-cflags.m4
• ... and 30 more: https://git.openjdk.org/jdk/compare/266e3d0decc09b9c17c455e2c754cd39114fa31a...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@stefank The following labels will be automatically applied to this pull request:

• hotspot
• shenandoah

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

[stefank]

The dual functionality of ReservedSpace makes the code hard to read and it is hard to see / realize that the constructors reserve memory. However, this is not the only reason to try to take a step back and clean up ReservedSpace. So while doing a restructure here I also want to clean up things like:

1. Move the _noaccess_prefix and related code out from ReservedSpace into ReservedHeapSpace.
2. Move _fd_for_heap out out of ReservedSpace.
3. Clearer requirements and asserts w.r.t. the input arguments size, alignment, and page_size.
4. ... and some more along the way while creating the patch. See the items below.

When reviewing this I would suggest that you compare the old code in virtualspace.cpp with the code in reservedSpace.cpp.

A guide to changes in the diff:

a) Whether memory reservation is for executable memory is confined to a limited number of places: The top-level memory reservation function and the function that reserves code. There's no need for other reservation places to pass in !ExecMem (or false), that is hidden from most callers.

b) The old code had shared helpers to handled the case when we map anonymous memory and the case when we mapped the heap to a file. I separated the two paths. This adds a small amount of code duplication, but IMHO it makes the code easier to read and it removes a bunch of dispatch if-statements in the lower level functions.

c) When starting with this patch I wanted to cleanup so that it would be slightly cleaner to pass down NMT MemTags, but I backed off because we need to make a couple of changes and fixes to NMT and CDS before that will be possible. Therefore you can see places where we don't pass down MemTags and you can see mtNone default values. My intention for this patch is to try to not change any of the places where NMT is called. Hopefully, we can get rid of the mtNone in the time frame of JDK 25.

d) G1 and Shenandoah used to call one ReservedSpace constructor that also changed the size of the requested memory. I've removed that convenience and let the GC fix the parameters themselves before calling the code that reserves memory. This allows us to have stricter argument validation checks. It also makes the reservation functions that we have left more consistent and therefore easier to understand also to call the right function.

e) As a future change, I would like to remove the memory reservation overload that helps the caller guess if they want large pages or not. There's a lot of confusion around that functionality, and I would prefer to let the callers that want to set up explicit large pages make that request explicitly instead. In fact, if you check the code today you could find a place where the GC code says that it doesn't want large pages, but if you follow the code you see that it still gets it. For now I have left this "capability" as-is.

f) I removed MemRegion* ReservedSpace::region(), since HeapWords* are so tied to the GC / Heap. An alternative could have been to move it to ReservedHeapSpace, but I don't think it is worth keeping that function.

g) You will see a few changes to the include files as a result of code moving.

h) I've stopped allowing 0 as an alignment argument. The required minimum alignment is os::vm_allocation_granularity(). Note, that we still have an overload for those who don't want to care about the alignment. That functions uses os::vm_allocation_granularity() under the hood. This allows for stricter checks and removes the code that changed alignment inside the reservation code.

i) ReservedSpace::release() used to clear the members of the Reservedspace instance. Since I have decoupled reserving/releasing from ReservedSpace. Code that want's to clear the ReservedSpace will do it explicitly.

j) I've somewhat restructured the compressed heap space reservation code. The previous code used its instance variables to keep track of previous attempts and didn't release failed reservation memory until the next attempt to reserve memory. I've moved the code to release the memory to be explicit instead. The code that reserves memory is now responsible to check if the returned ReservedSpace is good enough to use. Instead of repeatedly changing the instance members of ReservedHeapSpace, I let the code keep track of a local ReservedSpace that contain the latest reservation attempt. When all memory reservation attempts are done a noaccess prefix is created if needed and a ResevedHeapSpace is created and returned.

I've tested this by running the full tier1-7

[mlbridge]

Webrevs

• 02: Full - Incremental (c936bd44)
• 01: Full (7b053084)
• 00: Full (0721dc9d)

[jdksjolen]

Should release(release(R)) == release(R) and should release(R& r) do r = {}? Then release({}) is no-op.

In other words, instead of this:

if (rs.is_reserved()) {
  MemoryReserver::release(rs);
  rs = {};
}

we can have

MemoryReserver::release(rs);

which to me seems like a good simplification when using the API.

[jdksjolen]

It seems like you've explicitly decided against this in the new API, is there a particular reason for that?

[stefank]

My reasoning was that MemoryReserver does not have the ownership of the ReservedSpace instance, so I didn't want it to be the one clearing the members. If others feel the same way as you do, then I would prefer to add a function release_and_clear to make it clearer (:)) what's going on.

[jdksjolen]

release_and_cleardoes sound clearer :)

[stefank]

My preference is to stay with the current proposal and let code that requested reserve memory decide if and when the ReservedSpace should be cleared. I think it would be an easy thing to reevaluate in another RFE if we change our minds later on.

[jdksjolen]

"of the", "for the"

[afshin-zafari]

Thanks for this work.
I found no problem in logic and replacements. However, some Copyright years have to be updated:

• New files should have only one year in Copyright text:
  memoryReserver.?pp
reservedSpace.?pp

• Changed files:
  cds/dynamicArchive.hpp
gc/g1/g1PageBasedVirtualSpace.hpp
gc/parallel/psVirtualspace.hpp
gc/shared/generationCounters.cpp + .hpp
memory/metaspace/virtualSpaceNode.hpp

[stefank]

Thanks for this work.
I found no problem in logic and replacements.

Thanks for reviewing!

New files should have only one year in Copyright text:
memoryReserver.?pp
reservedSpace.?pp

I don't think this is correct. Code was copied from other files, so I transferred the copyright from those files. When I previously have asked around about this situation, the guidance I've been given the guidance that this is preferred over only using the current year.

[stefank]

@jdksjolen @afshin-zafari Are you OK with me pushing the code as it is right now?

[jdksjolen]

I am okay with this, I wanted to defer an approval to see whether others would chime in. Thank you for this work.

[afshin-zafari]

Thanks again for this work. No comments.

[stefank]

OK. Thanks. I'll give it one more day to let any reviewers get a chance to take a look.

[stefank]

I merged and ran tier1 internally and GHA. Thanks again for reviewing!
/integrate

[openjdk]

Going to push as commit 73b5dba.
Since your change was applied there have been 59 commits pushed to the master branch:

• d50b725: 8344647: Make java.se participate in the preview language feature requires transitive java.base
• 9e8aa85: 8346017: Socket.connect specified to throw UHE for unresolved address is problematic for SOCKS V5 proxy
• 5b703c7: 8342782: AWTEventMulticaster throws StackOverflowError using AquaButtonUI
• edbd76c: 8344951: Stabilize write barrier micro-benchmarks
• 842f801: 8339331: GCC fortify error in vm_version_linux_aarch64.cpp
• 4533109: 8345911: Enhance error message when IncompatibleClassChangeError is thrown for sealed class loading failures
• ea50c54: 8321818: vmTestbase/nsk/stress/strace/strace015.java failed with 'Cannot read the array length because "" is null'
• c0f0b8e: 8346151: Add transformer error logging to VerifyLocalVariableTableOnRetransformTest
• f3e2f88: 8346394: Bundled freetype library needs to have JNI_OnLoad for static builds
• 414eb6b: 8338714: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with JTREG_TEST_THREAD_FACTORY=Virtual
• ... and 49 more: https://git.openjdk.org/jdk/compare/266e3d0decc09b9c17c455e2c754cd39114fa31a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@stefank Pushed as commit 73b5dba.

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