RFR: JDK-8311035: CDS should not use dump time JVM narrow Klass encoding to pre-compute Klass ids
Thomas Stuefe
stuefe at openjdk.org
Fri Jun 30 06:19:54 UTC 2023
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" :-)
Hi @iklam,
thanks a lot for looking at this!
> 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
>
You missed a hunk without this patch makes no sense :)
https://github.com/openjdk/jdk/pull/14688/files#diff-ca37be6e6608d8db9e461dffd6574bc8a2dc3d1e0e88d9ba66735d4b37f8c226
I added a new function, `CompressedKlassPointers::initialize_for_given_encoding(klass range, encoding)`, that takes both the Klass range and encoding settings, and then sets up the encoding from these settings. This function complements the pre-existing `CompressedKlassPointers::initialize(klass range)`, that only gets a Klass range and then is free to think up any encoding it wants.
We call `CompressedKlassPointers::initialize_for_given_encoding` at CDS runtime now to re-instate base and shift. We call the old `CompressedKlassPointers::initialize()` when CDS is off, or (with this patch) at CDS dumptime.
I introduce this new function to remove CDS-specific paths from `CompressedKlassPointers`. The new function is a basically a replacement for the `if (UseSharedSpaces || DumpSharedSpaces) { ... }` path from the old `CompressedKlassPointers::initialize`.
About the "delta" constant: Its point was to codify the knowledge that the pre-computed encoding is calculated such that the future encoding base must be the same as the future mapping address (hence the 0). I'm fine with removing the constant though and replacing it with clear comments.
> 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 agree. We still could fail, but not here, and here we cannot deal with it.
We could still fail later if we mapped somewhere inconvenient due to ASLR, and the resulting Klass range start could not be used as an encoding base. That only can happen on Arm64 today. Today, we then assert, and this patch carries that assert over (at the start of the new `CompressedKlassPointers::initialize_for_given_encoding`).
As part of my Lilliput work, I also tackle that one. The JVM will then fall back to try a different base address and be smarter about it too. Then, as you have suggested offline, we could go and recompute the narrow Klass ids.
> 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" :-)
I agree.
Cheers, Thomas
-------------
PR Comment: https://git.openjdk.org/jdk/pull/14688#issuecomment-1614180709
More information about the hotspot-dev
mailing list