RFR: JDK-8311035: CDS should not use dump time JVM narrow Klass encoding to pre-compute Klass ids

[Thomas Stuefe]

On Wed, 28 Jun 2023 15:48:01 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> I recently spent a day trying to understand the interleaving of narrow Klass decoding with CDS. Thanks to @iklam for clarifying some details. This patch aims to make these interleavings easier to understand and more correct.
>> 
>> ----
>> 
>> CDS narrow Klass handling plays a role for archived heap objects. 
>> 
>> When dumping heap objects, we must recompute their narrow Klass ids since the relative positions of archived Klass instances change compared to their live counterparts in the dump time JVM.
>> We recompute those narrow Klass ids using the following encoding scheme:
>> - base = future assumed mapping start address
>> - shift = dump time (!) JVMs encoding shift  (A)
>> see `ArchiveHeapWriter::update_header_for_requested_obj` https://github.com/openjdk/jdk/blob/c3f10e847999ec254893de5a1a5de32fd07f715a/src/hotspot/share/cds/archiveHeapWriter.cpp#L419-L425
>> 
>> At CDS runtime, we load the CDS archive, then place the class space behind it. We then initialize narrow Klass encoding for the resulting combined Klass range such that:
>> - encoding base is the range start address (mapping base)
>> - encoding shift is always zero
>> see `CompressedKlassPointers::initialize` : https://github.com/openjdk/jdk/blob/c3f10e847999ec254893de5a1a5de32fd07f715a/src/hotspot/share/oops/compressedOops.cpp#L195-L231
>> 
>> The lengthy comment there is misleading and partly wrong (regrettable since it was mine :)
>> 
>> At dump time, when initializing the JVM, we also set the encoding base to klass range start and shift to zero (also `CompressedKlassPointers::initialize`). That is the shift we later use for (A); hence, that shift is zero.
>> 
>> -------------------
>> 
>> There are some small things wrong with the current code. That wrongness does not lead to errors but makes understanding the code difficult.
>> 
>> Mainly, the *dump time* VM's narrow Klass encoding scheme is irrelevant for the encoding employed on the future runtime archive since we recalculate the narrow Klass ids for archived heap objects. That means:
>> 
>> In `CompressedKlassPointers::initialize` there is no need to fix the encoding base and shift for the *dump time* JVM. Dump time JVM can use whatever base+shift it pleases; it can use the same code path as when CDS is off (e.g. use zero-based encoding).
>> 
>> In `ArchiveHeapWriter::update_header_for_requested_obj`, we should not use the dump time JVM shift for pre-computing the narrow Klass ids. Instead, we should use the *minimal shift needed for the maximal possible futu...
>
> Hi Thomas, thanks for doing the clean up.
> 
> I am not quite sure about the meaning of `precomputed_narrow_klass_base_delta`. Is it intended to be used for Lilliput, where a non-zero value may be used?
> 
> Since it's hard-coded to zero now, it's hard to observe how it would affect the calculation.
> 
> Before Liliput:
> 
> At dump time, all narrowKlass values are updated to be relative to `_requested_static_archive_bottom`, with `ArchiveHeapWriter::precomputed_narrow_klass_shift` (which is hard coded to zero)
> 
> At runtime, we always set `CompressedKlassPointers::base()` to `header()->mapped_base_address()` in here:
> 
> https://github.com/openjdk/jdk/blob/46e4ee1e80652203bd59d968ea72b27681bdf312/src/hotspot/share/cds/metaspaceShared.cpp#L1149
> 
> Currently it's impossible to set an alternative `CompressedClassSpaceBaseAddress` when using CDS:
> 
> 
>   develop(size_t, CompressedClassSpaceBaseAddress, 0,                       \
>           "Force the class space to be allocated at this address or "       \
>           "fails VM initialization (requires -Xshare=off.")                 \
> 
> 
> Also we will always have zero shifts, as we don't allow CompressedClassSpaceSize to be larger than 3G:
> 
> 
>   product(size_t, CompressedClassSpaceSize, 1*G,                            \
>           "Maximum size of class area in Metaspace when compressed "        \
>           "class pointers are used")                                        \
>           range(1*M, 3*G)                                                   \
>                                                                             \
> 
> 
> This means we can actually always use the archived Java objects, and this "if" block can be turned into an assert
> 
> 
>   if (archive_narrow_klass_base != CompressedKlassPointers::base() ||
>       narrow_klass_shift() != CompressedKlassPointers::shift()) {
>     log_info(cds)("CDS heap data cannot be used because the archive was created with an incompatible narrow klass encoding mode.");
>     return false;
> 
> 
> I think for the current implementation, we might as well get rid of these fields in filemap.hpp. 
> 
> 
>   size_t  _narrow_klass_base_delta;               // narrow Klass base and shift used to pre-calculate nKlass ids
>   int     _narrow_klass_shift;                    //  in archived objects (see comment in archiveHeapWriter.hpp)
> 
> 
> Having them there will create more confusion. They may be useful for the future, but it's hard to understand how these fields would be used without implementing the "future" :-)

@iklam : I removed the "delta" constant and replaced it with hopefully clear comments. I also removed the narrow_klass_xxx fields from FileMap. And translated runtime fail conditions to asserts in FileMapInfo::can_use_heap_region().

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

PR Comment: https://git.openjdk.org/jdk/pull/14688#issuecomment-1614217691