RFR: 8310160: Make GC APIs for handling archive heap objects agnostic of GC policy

[Ashutosh Mehra]

On Sat, 17 Jun 2023 17:21:05 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> This PR adds GC APIs to be implemented by all collectors for proper handling of archive heap space. Currently only G1 is updated to use these APIs which just involves renaming the existing G1 APIs.
>> In addition to that filemap.cpp is updated to replace calls to `G1CollectedHeap::heap()` with `Universe::heap()` to avoid G1 specific code as much as possible.
>> 
>> At many places in filemap.cpp heap range is requested from GC. All collectors except ZGC have contiguous heap and set `CollectedHeap::_reserved` to the heap range, so it can be easily exposed to the CDS code. This is done in this patch through `CollectedHeap::reserved` API. But for ZGC the heap can be discontiguous which makes it tricky to expose the heap range.
>> Another point to note is that most of the usage for heap range is for logging purpose, but there is one place where it is used for setting the `mapping_offset` in `FileMapInfo::write_region()` based on the heap start. So purely based on the functional requirement, we only need the heap start address, not the range.
>> 
>> To keep things simple and considering ZGC does not currently support archive heap, i refrained from tackling the issue of discontiguous heap range in this PR.
>
> src/hotspot/share/cds/filemap.cpp line 1570:
> 
>> 1568:       assert((mapping_offset >> CompressedOops::shift()) << CompressedOops::shift() == mapping_offset, "must be");
>> 1569:     } else {
>> 1570:       assert(UseG1GC, "used by G1 only");
> 
> Is this assert needed? If yes, it should be guarding the whole HeapShared::is_heap_region() branch, not just this else branch. OTOH we already assert "HeapShared::can_write()", which is a stronger assert.

Looking at it again, you are right. I don't think we need this assert. I will remove it.

> src/hotspot/share/cds/filemap.cpp line 2097:
> 
>> 2095:     assert(heap_range.contains(_mapped_heap_memregion), "must be");
>> 2096: 
>> 2097:     if (UseG1GC) {
> 
> This condition is odd. We only call FileMapInfo::map_heap_region() for G1, right? Maybe just assert at the start of the function for `ArchiveHeapLoader::can_map()` instead?

I mixed up my thoughts about the future changes where `FileMapInfo::map_heap_region()` could be called for any collector, not just G1. In that scenario the assert that mapped heap region is at the top of the heap would be true only for G1 (among current set of collectors that support archive heap).
Your point is valid. I can just add an assert `ArchiveHeapLoader::can_map()` at the start.

> src/hotspot/share/gc/shared/collectedHeap.hpp line 212:
> 
>> 210:   MemRegion reserved() const {
>> 211:     return _reserved;
>> 212:   }
> 
> PSHeap, GenCollectedHeap and Shenandoah each already expose `_reserved` via `reserved_region()`. Can we remove all of them and rename this to `reserved_region()`?

ok, I will do that.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14520#discussion_r1234169911
PR Review Comment: https://git.openjdk.org/jdk/pull/14520#discussion_r1234170044
PR Review Comment: https://git.openjdk.org/jdk/pull/14520#discussion_r1234171832