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

[Roman Kennke]

On Sat, 12 Aug 2023 11:52:58 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 incrementally with one additional commit since the last revision:
> 
>   fix windows

Hi Thomas,
nice change! I am not very familiar with the actual algorithms and platform specific details, but I have some suggestions regarding the structure.

src/hotspot/share/cds/metaspaceShared.cpp line 1332:

> 1330:     } else {
> 1331:       // Reserve anywhere, but mapping start address must be directly encodable as encoding base.
> 1332:       total_space_rs = Metaspace::reserve_address_space_for_compressed_classes(total_range_size, true);

Maybe add a /* cds_runtime */ comment before or after the 'true', so that it's clear what the value means? Or, even better, turn it into an enum?
Or even better, avoid it altogether? It looks to me as if the parts that are used when cds_runtime is false are all at the beginning of the method, so shouldn't be too hard to break out into its own method? And then chain or compose with the main body?

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

> 1811: // Given an address range [min, max), attempts to reserve memory within this area, with the given alignmend.
> 1812: // If randomize is true, the location will be randomized.
> 1813: char* os::attempt_reserve_memory_between(char* min, char* max, size_t bytes, size_t alignment, bool randomize) {

Could you avoid the bool parameter and use the flag RandomizeClassSpaceLocation directly? This means you would have to set that flag in tests, but that seems better than having boolean args there.
Also, it looks to me like the block that calculates the attach points (either randomized or not) Could be broken out into helper functions (one for randomized, one for non-randomized and one that dispatches between the two)?

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

Changes requested by rkennke (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15041#pullrequestreview-1578992544
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1294861752
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1294878664