RFR: 8296263: Uniform APIs for using archived heap regions

[Ashutosh Mehra]

On Mon, 7 Nov 2022 21:19:50 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> @iklam thanks for sharing the information and details on the future work in this space.
>> 
>>> By patching SharedStringsStress.java with this, I can get the CA1 and OA0 regions to be not aligned by GrainBytes, but that doesn't seem to cause the test to fail.
>> 
>> I was actually referring to CA0 and CA1 in my figures (which I realized was not clear in my explanation earlier). 
>> Anyway, I now understand the existing mechanism works fine because the following conditions are maintained (which you have already mentioned in your comment):
>> 1. G1 regions are at least 1MB, and are always a power of 2.
>> 2. At dump time the objects are placed such that they do not cross `HeapRegion::min_region_size_in_words()` which I believe is 1M.
>> 
>> Because of these two constraints, change in G1 region size at run time cannot result in objects crossing the region boundary.
>> So if I update the G1 code such that at run time the regions are mapped at 1M boundary then I can get rid of the problem of objects crossing region boundary and the two tests also pass.
>> 
>>> In any case, I think we can consider first changing the way the regions are written ([JDK-8296344](https://bugs.openjdk.org/browse/JDK-8296344)) so that they can be more easily mapped by various collectors.
>> 
>> I agree ([JDK-8296344](https://bugs.openjdk.org/browse/JDK-8296344)) would make it easier to map them at run time and would be happy to contribute to it anyway possible. But again, that's a GC policy specific implementation detail. 
>> I guess you would agree we need to de-couple the CDS code from the GC policy details. While JDK-8296344 aims at decoupling the code at dump time, my aim with this PR is to achieve the same at run time by having GC-agnostic APIs. 
>> Moreover, the dump time mechanism should not affect the APIs used for mapping regions at run time (though the implementation may need to be adjusted).
>> So, with this in mind do you think we can continue working on this PR, or do you believe the GC APIs this PR proposes to add would not be sufficient once JDK-8296344 is implemented?
>> 
>>> (Also, tactically, we should probably first change G1 to use the new "Uniform API" you are thinking about, but leave the other collectors unchanged. This way, we can gradually test things out and fix the other collectors in subsequent RFEs).
>> 
>> That makes sense. Ideally I should have done the implementation for other collectors in a separate RFEs. But I was worried if I the new APIs are flexible enough to support other non-G1 policies, and in an attempt to verify that I added the support for those policies as well. If it helps I can remove those commits and deliver them later in subsequent RFEs.
>
>> While JDK-8296344 aims at decoupling the code at dump time, my aim with this PR is to achieve the same at run time by having GC-agnostic APIs. Moreover, the dump time mechanism should not affect the APIs used for mapping regions at run time (though the implementation may need to be adjusted).
> 
> I think it depends on how we want to change the dump time operations. If we decide to go with a single contiguous block, then the API for mapping this block into the runtime heap will look very different than what you have today:
> 
> 
> bool ArchiveHeapLoader::get_heap_range_for_archive_regions(ArchiveHeapRegions* heap_regions, bool is_open) {
>   if (Universe::heap()->alloc_archive_regions(heap_regions->dumptime_regions(),
>                                            heap_regions->num_regions(),
>                                            heap_regions->runtime_regions(),
>                                            is_open)) {
> 
> 
> Also, we should probably record the region boundary information in the archived objects. Something like "objects never span across 1MB boundaries". This may need to be passed to the runtime mapping API, so incompatible collectors (i.e., one uses 512KB regions) can reject the archived objects.
> 
> One of my goal for JDK-8296344 is to optimize the archived objects for the collector chosen at dump time. For example, if you dump with SerialGC, the archived objects can be mapped efficiently without relocation when SerialGC is also chosen at run time, but may require relocation if G1 is chosen at run time. I am not sure if how this would affect the runtime mapping API. Maybe some sort of preference would need to be indicated.
> 
> I think it would be best for us to think about the whole picture before committing to a design. Timing wise, I think we missed the JDK 20 release anyway, so we should have plenty time to come up with a good design for JDK 21.
> 
> I also would like to hear from folks in our GC team. @tschatzl @stefank

@iklam you mentioned [here](https://github.com/openjdk/jdk/pull/10970#issuecomment-1302600242) that you were not able to run the tests with -agentvm option using fastdebug build. Its a long shot but I am wondering if you still remember what error you were getting and which tests you were running.

I recently tried running in agentvm mode with this PR and tests ran fine. The command I used was:

`java -jar <path to jtreg.jar> -agentvm -verbose:all -jdk:<path to jdk to test> test/hotspot/jtreg:hotspot_cds`

Some tests (like `runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java`) had this error `Use -nativepath to specify the location of native code` but I see the error without this patch as well.

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

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