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

[Thomas Stuefe]

On Tue, 15 Aug 2023 16:41:22 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix windows
>
> 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?

That's why I favored my original name for that parameter, "strict_base", since it much clearer described behavior than "cds_runtime" does.

As for split-up, I rather avoid it. The resulting parts would be difficult to describe and overlap in functionality. The cds_runtime path, including this parameter, will hopefully be removed soon when we implement recalculating the nKlassIDs at CDS runtime.

I will add a clear comment.

> 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)?

`os::attempt_reserve_memory_between` is a generic API, usable for other things too. One obvious use case would be allocating the heap (replacing all that manual lets-reserve-the-heap-in-lower-regions in virtualSpace.cpp); another one Shenandoah CollectionSet; and some more. I don't want to impose the "randomize" decision on all callers.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1295424577
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1295426138