RFR: 8345655: Move reservation code out of ReservedSpace

[Stefan Karlsson]

On Thu, 12 Dec 2024 13:36:07 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

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

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

-------------

PR Comment: https://git.openjdk.org/jdk/pull/22712#issuecomment-2539001078