RFR: JDK-8312018: Improve reservation of class space and CDS [v8]

[Andrew Dinn]

On Tue, 29 Aug 2023 06:01:41 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> This PR rewrites memory reservation of class space and CDS to have ASLR and a much better chance of low-address placement.
>> 
>> ------
>> 
>> Motivation:
>> 
>> (I was advised to keep PR text short lest I spam the mailing lists with Skara generated mails. So, for motivation, see JBS issue)
>> 
>> -------
>> 
>> The patch introduces a new API to reserve memory within an address range at a randomized location, while trying to be smart about it. The API is generic, and future planned uses of this API could include replacing the zero-based heap allocation and the zero-based reservation of Shenandoah Collection Sets, thereby allowing us to consolidate coding.
>> 
>> This PR complements @iklam 's current work that rewrites archive heap initialization at runtime. Once his work is in, we will be able to recalculate narrow Klass IDs for objects loaded from the archive, and that will allow us to reap the benefits of this patch for the CDS runtime case too.
>> 
>> -------
>> 
>> Noteworthy functional changes:
>> 
>> - class space is now likely to be reserved at a random location in low address ranges; this now includes ranges below 2 GB, which had been excluded before due to the use of HeapBaseMinAddress as minimal attach point.
>>      -  Note that this makes no problem with sbrk() - the only platform still having this problem is AIX, and we solved it differently there, see comment in AIX implementation of `os::vm_min_address()`
>> - I removed the PPC/AARCH64 specific coding that attempted to map at 4G/32G aligned addresses. That section has a complex history - it was originally introduced to deal with AARCH64 immediate-loading shortcomings, but PPC piggybacked on it for its perceived ability to allocate for zero-based, which got subsequently lost, so its broken now (see https://bugs.openjdk.org/browse/JDK-8313669). The new code is a better replacement for this coding.
>> 
>> -------
>> 
>> Example (linux amd64):
>> 
>> We start the JVM with a 30GB heap. 
>> 
>> In the stock JVM, the JVM will place the heap in the lower address ranges starting at 2G (0x8000_0000). But then it is unable to place the class space in lower regions too, so it placed it at 32 GB (0x8_0000_0000), and we don't have zero-based encoding (Narrow klass base: 0x0000000800000000). This scenario repeats for every iteration, so we will always use these two addresses (no ASLR): 
>> 
>> 
>> thomas at starfish $ ./images/jdk/bin/java -Xshare:off -Xmx30g -Xlog:gc+heap+exit -Xlog:gc+metaspace -version
>> [0.019s][info][gc,metaspace] ...
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits:
> 
>  - merge
>  - restrict 16-bit-move-optimized code path to platforms that benefit from it
>  - Fix test bug on machines with >47 bit address space (ppcle and linux aarch64 /w 52 bit AS
>  - Feedback Ioi
>  - Roman: better comment in metaspaceShared
>  - Changes to os::vm_min_addr
>  - fix windows
>  - added temporary logging for ppc
>  - make API smaller for Ioi
>  - factor out shuffle and hemi-split
>  - ... and 4 more: https://git.openjdk.org/jdk/compare/3dc266c5...a213a375

Nice work.

src/hotspot/share/memory/metaspace.cpp line 597:

> 595:   // the encoding base to zero anyway: the encoding base has to be the base of the mapped archive
> 596:   // (ultimately, the start address of the region we are reserving here).
> 597:   const bool try_in_low_address_ranges = !cds_runtime;

I think `try_in_low_address_range` would be a far better name for the method param than either the current `cds_runtime` or your original `strict_base`, following the principle that *it does what it says on the tin*.

src/hotspot/share/runtime/os.cpp line 1858:

> 1856: }
> 1857: 
> 1858: // Given an address range [min, max), attempts to reserve memory within this area, with the given alignmend.

... with the given *alignment*.

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

Marked as reviewed by adinn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15041#pullrequestreview-1600040121
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1308563477
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1308564521