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

[Thomas Schatzl]

On Fri, 2 Jun 2023 07:27:17 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>>> @tschatzl
>>> 
>>> > I'm not convinced not giving a preferred location is a good idea. That seems to reduce the opportunity to directly map archives significantly. Previously, with only heap size changes, the archive could be mapped in still.
>>> 
>>> I am not sure I get this. The patch does not change the ability to map the archive. It just moved the calculation to map the archive region from CDS to G1. Before this patch CDS code would determine the address for mapping the archive space towards the top of the heap and pass that address to G1. This patch just moves that calculation to G1. So it should be at-par with the current state. If it is not, please point out and I can work on fixing that.
>>> 
>>> > Since this change is an intermediate step, could you provide an overview of the final API/change too? It is hard to comment on this without knowing where you are going with that.
>>> 
>>> Ok, I have updated the description of the main issue [JDK-8296263](https://bugs.openjdk.org/browse/JDK-8296263) with some details on the expected changes in the future patches. My main aim with this work is mainly code reorganization to avoid using different GC APIs in CDS depending on the GC policy in use. When this is completed I expect it to provide same functionality as today. Any enhancement, like passing preferred location to map archive heap, can be built on top of this.
>>> 
>>> Hope this helps.
>> 
>> Hi Ashutosh,
>> 
>> You are right that in the existing code, although filemap.cpp finds out where the requested range is, it doesn't actually pass that to G1. It just requests G1 to reserve a range at the end of the runtime heap. So your PR preserves this behavior.
>> 
>> I think Thomas's point is, the requested range should be passed to the `alloc_archive_regions()` API, even though the collector may simply ignore it.
>> 
>> In the past, the archive G1 regions were not movable, so it was preferable to put them at the end of the heap, even though that might cause relocation. Now that the archive regions are just regular "old" regions, which can move, it may be preferable to reserve them at the requested range.
>> 
>> BTW, perhaps `alloc_archive_regions()` should be renamed to `alloc_archive_range()` going forward.  The plural form of "regions" sounds odd for non-region based collectors.
>
>> Hi Ashutosh,
>> 
>> You are right that in the existing code, although filemap.cpp finds out where the requested range is, it doesn't actually pass that to G1. It just requests G1 to reserve a range at the end of the runtime heap. So your PR preserves this behavior.
>> 
>> I think Thomas's point is, the requested range should be passed to the `alloc_archive_regions()` API, even though the collector may simply ignore it.
> 
> Exactly, sorry if I wasn't clear enough.
> 
>> 
>> In the past, the archive G1 regions were not movable, so it was preferable to put them at the end of the heap, even though that might cause relocation. Now that the archive regions are just regular "old" regions, which can move, it may be preferable to reserve them at the requested range.
>> 
>> BTW, perhaps `alloc_archive_regions()` should be renamed to `alloc_archive_range()` going forward. The plural form of "regions" sounds odd for non-region based collectors.
> 
> +1
> 
> Thanks,
>   Thomas

> @tschatzl let me know if there are any other concerns to address in this patch.

Given the API is in flux, and the follow-up not certain to be reviewed by Thursday I would like to have this patch *at least* moved to after the JDK 21 fork. Otherwise maintainers need to deal with this awkwardness for a long time unnecessarily.

Unlike Ioi I am not good with the current naming.

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

PR Comment: https://git.openjdk.org/jdk/pull/14208#issuecomment-1578693782