RFR: 8309065: Move the logic to determine archive heap location from CDS to G1 GC [v3]

[Thomas Stuefe]

On Fri, 2 Jun 2023 19:46:31 GMT, Ashutosh Mehra <duke at openjdk.org> wrote:

>> This patch is the first step towards having a single set of GC APIs for allocating heap space for the archived objects (See https://bugs.openjdk.org/browse/JDK-8296263).
>> It moves some of the G1 specific logic from CDS to G1 gc without changing the functionality.
>> 
>> Changes that add/update GC APIs for handling archive heap would be introduced in upcoming patches.
>
> Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments - updates to alloc_archive_regions() api
>   
>   Signed-off-by: Ashutosh Mehra <asmehra at redhat.com>

This looks okay, it is a cleaner separation than before.

Small nits and questions only. Approved (not for jdk21 and pending @tschatzl approval)

One note, I don't see a clear usage of INCLUDE_CDS and INCLUDE_G1GC, and it predates this patch. Don't we do this anymore? We still want to be buildable without CDS but with G1, and vice versa, right?

src/hotspot/share/cds/filemap.cpp line 2124:

> 2122: 
> 2123:   size_t word_size = size / HeapWordSize;
> 2124:   address requested_start = heap_region_requested_address();

Possibly for another RFE, if you intent to make this code move as verbatim as possible:

This feels like something that should use `r->mapping_offset()` directly - would make the code easier to understand.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 551:

> 549:   HeapWord* start_addr = reserved.end() - align_up(word_size, HeapRegion::GrainWords);
> 550:   MemRegion range = MemRegion(start_addr, word_size);
> 551:   HeapWord* last_address = range.last();

Do I understand this - and the old code - correctly: we map the archive region at the end of the heap, therefore, if heap size changed between dump time and runtime, we will always have to relocate? I didn't look but I assume the dumptime heap size is the default heap size?

Another, possibly stupid, question: why don't we use the Heap bottom - we'd still avoid fragmentation but have a much higher chance of not relocating?

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 712:

> 710:   // the location of the archive space in the heap. The returned address may or may
> 711:   // not be same as the preferred address.
> 712:   HeapWord* alloc_archive_region(size_t word_size, HeapWord* preferred_addr);

The comment s good. This is only supposed to be used at JVM start, CDS runtime, before we start actually using the heap, right? Could we add this limitation to the comment for the casual reader?

Also, here and in other places G1: guard with `#ifdef INCLUDE_CDS` ?

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

Marked as reviewed by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14208#pullrequestreview-1467074758
PR Review Comment: https://git.openjdk.org/jdk/pull/14208#discussion_r1221212920
PR Review Comment: https://git.openjdk.org/jdk/pull/14208#discussion_r1221241979
PR Review Comment: https://git.openjdk.org/jdk/pull/14208#discussion_r1221218276