[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8297313: Refactor APIs for calculating address of CDS archive heap regions
From:       Calvin Cheung <ccheung () openjdk ! org>
Date:       2022-11-30 17:25:00
Message-ID: XExj3b7HcgCRwMFxczsqvKKGgRxIXWNGDYYmXbi2GQ0=.7d778546-106b-46d3-b4c1-f2a689038110 () github ! com
[Download RAW message or body]

On Mon, 21 Nov 2022 06:22:49 GMT, Ioi Lam <iklam@openjdk.org> wrote:

> In anticipation of the following RFEs:
> 
> - [JDK-8296263](https://bugs.openjdk.org/browse/JDK-8296263): Uniform APIs for \
>                 using archived heap regions
> - [JDK-8296344](https://bugs.openjdk.org/browse/JDK-8296344): Remove dependency on \
> G1 for writing the CDS archive heap 
> We need to clean up the APIs for calculating the addresses of CDS archive heap \
> regions.  The current APIs have confusing names and the implementation is \
> convoluted. 
> The proposal is to change the above to three APIs with easy-to-understand \
> semantics: 
> // The actual address of this region during dump time.
> address heap_region_dumptime_address(FileMapRegion* r)
> 
> // The address where this region can be mapped into the runtime heap without
> // patching any of the pointers that are embedded in this region.
> address heap_region_requested_address(FileMapRegion* r)
> 
> // The address where this shared heap region is actually mapped at runtime.
> address heap_region_mapped_address(FileMapRegion* r)
> 
> 
> The meaning of the `CDSFileMapRegion::_mapping_offset` field is changed slightly to \
> simplify the implementation of the above 3 functions. Comments are improved explain \
> what's going on. 
> With the above changes, `FileMapInfo::map_heap_regions_impl()` can also be \
> simplified significantly. 
> ****
> Testing: tiers 1-4
> ****
> Misc changes:
> - added more assertions about exactly which combinations of collector and oop \
>                 encoding mode are supported
> - Specifically, uncompressed oops for non-G1 collectors are currently not \
>                 supported;
> - Change error message accordingly when the user selects an unsupported \
> combination.

Looks good. One nit below.

src/hotspot/share/cds/archiveHeapLoader.hpp line 144:

> 142:   static bool      _mapped_heap_relocation_initialized;
> 143: 
> 144:   static void init_narrow_oop_decoding(address base, int shift) \
> NOT_CDS_JAVA_HEAP_RETURN;

This is already inside the `#if INCLUDE_CDS_JAVA_HEAP` block. So the \
`NOT_CDS_JAVA_HEAP_RETURN` seems redundant.

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

Marked as reviewed by ccheung (Reviewer).

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


[prev in list] [next in list] [prev in thread] [next in thread]