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

[Ashutosh Mehra]

On Fri, 2 Jun 2023 20:10:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> 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>
>
> 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);
> 
> Sorry to be picky, but I think `region` implies a single region, but G1 could allocate one or more regions to satisfy the request. I think it's better to use `allocate_archive_range(MemRegion requested_range)` to be more neutral. Passing the range in a `MemRegion` will also look similar to the API right above this one.
> 
> Maybe we should also change `populate_archive_regions_bot_part` and `dealloc_archive_regions` to use `_range` as well. What do you think, @tschatzl

How about replacing `allocate_archive_range` with  `allocate_archive_space` which is actually planned for the next patch?
I am more inclined to keep size and address as separate parameters because the address is just a hint for the collectors, while size is not. With MemRegion as the type this information is not conveyed to the reader without reading the comments/code. This is also the reason why I prefer using "preferred_addr" rather than "requested_addr".
But if you feel otherwise I will update the API to use MemRegion.

> Maybe we should also change populate_archive_regions_bot_part and dealloc_archive_regions to use _range as well

I am not sure if it is necessary to update these APIs, as I would anyway be replacing them with fixup_archive_space() and handle_archive_space_failure() as mentioned in the description of main issue [JDK-8296263](https://bugs.openjdk.org/browse/JDK-8296263). IMO these names are more generic. If you want I can update the code to use these new names in this patch.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14208#discussion_r1214819384