RFR: 8265268: Unify ReservedSpace reservation code in initialize and try_reserve_heap

[Thomas Schatzl]

On Mon, 19 Apr 2021 13:51:19 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

> Please review this change to unify the reservation code in ReservedSpace. 
> 
> **Summary**
> The code in `ReservedSpace::initialize(...)` and `ReservedHeapSpace::try_reserve_heap(...)` is very similar and more or less does the same things. The difference between them is logging, what is considered a failure and how that is handled.
> 
> This change introduces a new function `ReservedSpace::reserve(...)` that is used by both `initialize(...)` and `try_reserve_heap(...)`. The biggest reason for only having one function handling the reservations is to avoid code duplication and having to change two places when something needs to be updated.
> 
> There will be some small changes in what is getting logged in some cases, but the goal is to avoid changing behavior as much as possible. 
> 
> Some additional notes to help reviewing:
> * There are two reservation helpers:
>   `reserve_memory()` for regular pages and file backed mappings.
>   `reserve_memory_special()` for explicit large pages.
> * In `reserve_memory()` the `is_align()`-check at the end was previously done at the end of `initialize()`. It was moved here because this is the only case where an unaligned address can be returned. For large pages as well as when a requested address is provided, the returned address will be aligned. 
> * The support to have a backing file is only used by the heap when `AllocateHeapAt` is set. If this option is used together with `UseLargePages` a debug message is printed since it is up to the backing file system if large pages are used or not. This output has been unified and is now printed when the file is created rather then deep in the reservation code. Previously this message differed depending on if `UseCompressedOops` was enabled or not. Now the same message is printed for both cases.
> 
> * The failure checking function `failed_to_reserve_as_requested(...)` now only checks if the memory was allocated at the expected address. Releasing the memory is done by the caller. 
> 
> * The size passed into `map_or_reserve_memory_aligned` was previously aligned up using the alignment, but the size should always be aligned already. An assert has been added to the start of the `reserve` function and the `align_up` has been removed.
> 
> **Testing**
> A lot of local testing as well as tier1-tier5 in Mach5.

Changes requested by tschatzl (Reviewer).

src/hotspot/share/memory/virtualspace.cpp line 146:

> 144:   char* base;
> 145:   // If the memory was requested at a particular address, use
> 146:   // os::attempt_reserve_memory_at() to avoid over mapping something

"to avoid mapping over" - word order

src/hotspot/share/memory/virtualspace.cpp line 147:

> 145:   // If the memory was requested at a particular address, use
> 146:   // os::attempt_reserve_memory_at() to avoid over mapping something
> 147:   // important.  If available space is not detected, return NULL.

"If available space is not detected" -> "If this (reservation) is unsuccessful...".

available space detected != reservation possible/successful

src/hotspot/share/memory/virtualspace.cpp line 217:

> 215:   // There are basically three different cases that we need to handle below:
> 216:   // - Mapping backed by a file
> 217:   // - Mapping backed by explicit large pages

I think there should be a description what "explicit large pages" are. Only in the case that the OS can't commit large page memory this path is taken.
The third option kind of suggests that there is only "explicit large page support" if you want large pages (only talking about normal pages or THP, but on e.g. Linux you do not need to pre-commit large pages reservations.

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

PR: https://git.openjdk.java.net/jdk/pull/3570