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

[Thomas Stuefe]

On Fri, 16 Jun 2023 14:26:09 GMT, Ashutosh Mehra <duke 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.

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?

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

src/hotspot/share/gc/shared/collectedHeap.hpp line 533:

> 531:   // GC policy can take necessary actions to make the archive space usable,
> 532:   // such as inserting filler objects to make it parseable, or updating block offset table.
> 533:   virtual void fixup_archive_space(MemRegion range) { return; }

Small nit about the naming of these callbacks. "fixup_archive_space" implies knowledge about what the collector does, as does "handle_archive_space_failure". But you want to abstract away collector work from CDS. @iklam already remarked on "handle..". 

I see these methods both more as callbacks, and as such they could be named after the point at which they are called, e.g. "on_cds_post_load" and "on_cds_load_error". Or similar. Named after when they are called, but assuming no knowledge of what the collector does at that point, if anything at all.

Up to you. This is very bikesheddy, I'm also fine with the names as they are.

src/hotspot/share/gc/shared/collectedHeap.hpp line 536:

> 534: 
> 535:   // Handle any failure in loading the CDS archive area.
> 536:   virtual void handle_archive_space_failure(MemRegion range) { return; }

For both `fixup_archive_space` and `handle_archive_space_failure`: Why do we need to pass in the MemRegion? The only valid argument in both cases is the MemRegion that had been previously established by a call to `alloc_archive_space`. 

Why not let CollectedHeap remember the MemRegion established by `alloc_archive_space`, thereby also guaranteeing that `alloc_archive_space` can only be called once, and refer to that in the followup calls?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14520#discussion_r1233097531
PR Review Comment: https://git.openjdk.org/jdk/pull/14520#discussion_r1233101564
PR Review Comment: https://git.openjdk.org/jdk/pull/14520#discussion_r1233094909
PR Review Comment: https://git.openjdk.org/jdk/pull/14520#discussion_r1233100361
PR Review Comment: https://git.openjdk.org/jdk/pull/14520#discussion_r1233100119