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

[Thomas Stuefe]

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 explains this in greater detail. Thankfully, that minimal shift is exactly zero in today's hotspot.

Similarly, we should not save the dump time JVMs encoding shift as the "narrow Klass shift" into the archive, but the one we use for pre-computing the narrow Klass ids.

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

The patch makes the encoding scheme used for precomputing klass ids explicit and obvious by introducing two new constants, `ArchiveHeapWriter::precomputed_narrow_klass_shift` and `ArchiveHeapWriter::precomputed_narrow_klass_base_delta`.

The former is self-explanatory. The latter intends to make it clear that nKlass IDs are relative, so what matters is not the actual value for base used to pre-compute those ids, but its *distance to the Klass range start*. In other words, the archive can be mapped at any address, and the narrow Klass IDs would be still usable as long as `(<runtime mapping address> - delta) == <runtime encoding base>`. Since we currently require the mapping base to be the encoding base, the delta is 0.

(Reviewer opinion: is that too complex? Am I overthinking? I wanted to make the fact that we use a delta of zero perfectly obvious; since Klass range start and encoding base are semantically not the same. OTOH, a delta value != 0 will rarely make sense outside of very limited corner cases (running with pre-computed zero-based klass ids).

I also store the delta used to precompute the base in the header, and keep the shift in the header.

(Reviewer opinion: Alternatively, we could remove these two values from the filemap header completely and just use the new constants throughout the code base. That would be simpler and better grep'able.  Opinions?)

Finally, I changed the initialization of narrow Klass encoding such that:
- at dump time, we don't use a fixed base and shift
- I split the initialize functions into two, with the new variant only used at runtime by CDS when we want to re-instate a given encoding. 

The biggest functional change apart from FileMap header stuff is the fact that the dump time JVM now is free to choose its base+shift.

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

Commit messages:
 - fix-cleanup-CDS-nKlass-encoding

Changes: https://git.openjdk.org/jdk/pull/14688/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14688&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8311035
  Stats: 137 lines in 8 files changed: 65 ins; 44 del; 28 mod
  Patch: https://git.openjdk.org/jdk/pull/14688.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14688/head:pull/14688

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