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

[Ioi Lam]

On Tue, 4 Jul 2023 05:31:09 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> I recently spent too much time trying to understand the interleaving of narrow Klass decoding with CDS. Thanks to @iklam for clarifying some details. This patch aims to make the code 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 minor things wrong with the current code. 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. Tha...
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits:
> 
>  - Merge branch 'master' into fix-cleanup-CDS-nKlass-encoding
>  - revert accidental change
>  - Fix Windows build
>  - Add alternative for !INCLUDE_CDS_JAVA_HEAP path
>  - Merge branch 'master' into fix-cleanup-CDS-nKlass-encoding
>  - fix comment
>  - Merge
>  - -remove narrow_klass_xxx from FileMap
>    - remove ArchiveHeapWriter::precomputed_narrow_klass_base_delta and replaced it with clear comments
>    - changed runtime fail condition to asserts in FileMapInfo::can_use_heap_region()
>  - fix-cleanup-CDS-nKlass-encoding

Looks reasonable to me. This is a nice clean up. Just a few nits.

src/hotspot/share/cds/archiveHeapWriter.hpp line 205:

> 203:   //    Since nKlass itself is 32 bit, our encoding range len is 4G, and since we set the base directly
> 204:   //    at mapping start, these 4G are enough. Therefore, we don't need to shift at all (shift=0).
> 205:   static constexpr int precomputed_narrow_klass_shift = 0;

"future" and "runtime" seem to mean the same thing. Maybe we should consistently use "runtime"?

src/hotspot/share/oops/compressedOops.cpp line 192:

> 190: // set this encoding scheme. Used by CDS at runtime to re-instate the scheme used to pre-compute klass ids for
> 191: // archived heap objects.
> 192: void CompressedKlassPointers::initialize_for_given_encoding(address addr, size_t len, address requested_base, int requested_shift) {

This entire function should be inside _LP64. It should not be referenced by a 32-bit build.

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

Marked as reviewed by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14688#pullrequestreview-1513680142
PR Review Comment: https://git.openjdk.org/jdk/pull/14688#discussion_r1252552155
PR Review Comment: https://git.openjdk.org/jdk/pull/14688#discussion_r1252557753