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

[Ioi Lam]

On Wed, 28 Jun 2023 07:48:33 GMT, Thomas Stuefe <stuefe 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 future Klass range size*. The comment in ArchiveHeapWriter expla...

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" :-)

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

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