RFR: 8296263: Uniform APIs for using archived heap regions

[Thomas Schatzl]

On Thu, 3 Nov 2022 16:06:47 GMT, Ashutosh Mehra <duke at openjdk.org> wrote:

> This is an attempt to unify the two different approaches for using archived heap regions. Main goal is to restructure and modify the code to have a single set of GC APIs that can be called for using archived heap regions.
> 
> In current state, the VM either tries to "map" (for G1) or "load" (for non-G1 GC policies) the archived heap regions into the java heap.
> When mapping, the VM determines the address range in the java heap where the archived regions should be mapped. It tries to map the regions towards the end of the heap. The APIs used for this purpose are G1 specific.
> When loading, the VM asks the GC to provide a chunk of memory from the heap, into which it reads the contents of the archived heap regions. The APIs used are GC policy agnostic but challenging to use for region based collectors.
> 
> This PR attempts to add new set of GC APIs that can be used by the VM to reserve space in the heap for mapping the archived heap regions. It combines the good parts of the two existing approaches. Similar to the "loading" API, in this new approach VM is not responsible for determining the mapping address. That responsibility always resides with the GC policy. This also allows the flexibility for the GC implementation to decide where and how to reserve the space for the archived regions. For instance, G1 implementation can continue to attempt to allocate the space towards the end of the heap.
> This PR also provides the implementation of the new APIs for all the existing GC policies that currently support archived heap regions viz G1, serial, parallel and epsilon.

Some additional initial notes about the code:

* please document the new APIs in `CollectedHeap` (something like "// Support for mapping archive regions into the heap" for 6 methods with tons of parameters is not sufficient) - it would also help reviewing.
* also we should be careful with the naming as the term "region" is overloaded enough already; e.g.  `heap_region_dealloc_supported` seems to miss an `archive`.
* while we are at changing the API, the change should use appropriate types, i.e. unsigned ints/size_t for sizes.
* there is quite a bit inconsistency in the naming: sometimes the MemRegions are called "something_regions", sometimes "something_ranges", there is also "something_spaces"; the same with the associated counts that are sometimes called "count", and sometimes "num_regions".

Summarizing all this, I would strongly suggest to use the term "ranges" instead of regions for the areas/MemRegions from the archive. This would help a lot with code clarity in collectors that already use the term regions.

Similar to @iklam I strongly suggest splitting this change by collector (or at least basic+g1 and epsilon/serial/parallel) anyway. There will likely be considerable changes on top of your current stack of changes which may require working/reviewing on the tip.

There are additional initial comments in the PR. I understand that some of them may become obsolete as we change the API.

I did not really look at dumptime considerations which seem to be under discussion right now; I do not have the overview about the exact requirements/problems in that area, particularly with regards to JDK-8296344.
The chosen API (and the intended execution flow during dump/runtime) is underdocumented at the moment to give good comments. It would be nice to summarize that to start a discussion; maybe even discuss this on some mailing list instead in the PR of a 2k LOC change.

However, from the existing commetns here, I do think it is a good idea to write out all chosen assumptions when dumping (e.g. the alignment boundary of 1MB), even if currently that is the minimum for all collectors.

src/hotspot/share/cds/archiveHeapLoader.cpp line 100:

> 98:                                            heap_regions->runtime_regions(),
> 99:                                            is_open)) {
> 100:     heap_regions->set_state(ArchiveHeapRegions::HEAP_RESERVED);

Indentation

src/hotspot/share/cds/archiveUtils.cpp line 153:

> 151:   _dumptime_regions = MemRegion::create_array(max_region_count, mtInternal);
> 152:   _runtime_regions = MemRegion::create_array(max_region_count, mtInternal);
> 153:   _region_idx = (int *)os::malloc(sizeof(int) * max_region_count, mtInternal);

Use `NEW_C_HEAP_ARRAY/FREE_C_HEAP_ARRAY` instead of directly using `os` methods.

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

> 2119:                 UseCompressedOops ? p2i(CompressedOops::end()) :
> 2120:                                     UseG1GC ? p2i((address)G1CollectedHeap::heap()->reserved().end()) : 0L);
> 2121: #endif

Should be dropped.

src/hotspot/share/gc/epsilon/epsilonHeap.cpp line 157:

> 155: 
> 156:   assert(alignment == 0 || is_aligned(res, alignment), "Allocated space " PTR_FORMAT
> 157:            " does not have requested alignment of " SIZE_FORMAT " bytes", p2i(res), alignment);

Indentation

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

> 539: }
> 540: 
> 541: bool G1CollectedHeap::alloc_archive_regions(MemRegion* dumptime_regions, int num_regions, MemRegion* runtime_regions, bool is_open) {

I'm not completely clear why we need both dumptime and runtime ranges here. I can see that dumptime range size is used for getting sizes, and runtime ranges are then updated to reflect the actual mapping.

Please do not intermix input/output parameters in the parameter list unless absolutely necessary. Additionally, documentation in `CollectedHeap` would have saved me an hour or so trying to understand the code.

I.e. the signature of `alloc_archive_regions` should be:

bool G1CollectedHeap::alloc_archive_regions(MemRegion* dumptime_regions, int num_regions, bool is_open, MemRegion* runtime_regions) {

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

> 553:            "is not aligned to OS default page size", region_size);
> 554:     total_size += region_size;
> 555:   }

Factor this out into a helper method somewhere; this code seems to be duplicated in all implementations too. Also some effort should be expended to factor out common code between collectors.
The code for Epsilon/Serial/Parallel looks at least from the outside apart from minor differences like copy&paste.

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

> 557:   HeapRegion* hr = NULL;
> 558:   HeapWord* mem_end = NULL;
> 559:   HeapWord* mem_begin = NULL;

Please use `nullptr` in new code everywhere instead of `NULL`.

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

> 575:       }
> 576:       return false;
> 577:     }

This big loop could be moved out of this huge method by having `alloc_highest_free_region` take a number of contiguous regions to allocate; this would also remove the need for the bailout uncommits as `alloc_highest_free_region` at the lowest level can simply search for a contiguous range of bits instead of a single bit, and reserve the whole range at once.

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

> 591:   mem_begin = hr->bottom();
> 592: 
> 593:   HeapRegion* prev_range_last_region = NULL;

This block of code should have a high-level comment about what it does:
* filling in runtime region information (start/end)
* updating G1 region information (tops)/adding to archive set/printing allocation

Even better, move this loop into a well-named (static?) helper method; this method is just way too long.

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

> 601:       curr_range->set_start(align_up(runtime_regions[i-1].end(), alignment));
> 602:     }
> 603:     assert(is_aligned(curr_range->start(), alignment), "region does not start at OS default page size");

s/region/MemRegion/ or "range"

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

> 606:     // Add each G1 region touched by the range to the old set, and set the top.
> 607:     HeapRegion* curr_region = _hrm.addr_to_region(curr_range->start());
> 608:     HeapWord* last_address = curr_range->last();

s/old set/archive set/

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

> 612:       if (curr_region != prev_range_last_region) {
> 613:         _hr_printer.alloc(curr_region);
> 614:         _archive_set.add(curr_region);

I think both statements should be moved where the allocation and setting the region type happens. Put all these updates to the region into a (static?) helper method.

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

> 629: }
> 630: 
> 631: bool G1CollectedHeap::check_archive_addresses(MemRegion* ranges, size_t count) {

Please be consistent with parameters, i.e. ranges vs. regions, count vs. num_whatever

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

> 670: }
> 671: 
> 672: void G1CollectedHeap::populate_archive_regions_bot_part(MemRegion* ranges, size_t count) {

These two methods should be merged into one. `complete_archive_regions_alloc` is the only caller of `populate_archive_regions_bot_part` apparently.

Again please check parameter names for consistency.

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

> 694:   assert(!is_init_completed(), "Expect to be called at JVM init time");
> 695:   assert(ranges != NULL, "MemRegion array NULL");
> 696:   assert(num_regions != 0, "No MemRegions provided");

The method could probably just find out all regions that are covered by the given ranges the same way as `alloc_archive_regions` did (i.e. summing up byte-sizes; btw, parameters contain both "ranges" and "regions") and uncommit them en-bloc.
This seems to go range by range and find out which regions they correspond to and uncommit them one by one.

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

> 704:   // notify mark-sweep that the range is no longer to be considered 'archive.'
> 705:   MutexLocker x(Heap_lock);
> 706:   for (int i = 0; i < num_regions; i++) {

Don't use ints for counts.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 816:

> 814:   }
> 815: 
> 816:   // Cannot use verbose=true because Metaspace is not initialized

That comment is confusing.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 828:

> 826:       curr_range->set_start(result);
> 827:     } else {
> 828:       // next range should be aligned to page size

Sentences in comments should start with upper case and end with a full stop (I'm only commenting this once here).

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

Changes requested by tschatzl (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10970