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

[tstuefe]

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.

(2023-07-05 Updated to fit patch description to the agreed final form)

---

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

  jdk/src/hotspot/share/cds/archiveHeapWriter.cpp

  Lines 419 to 425 in c3f10e8

  	void ArchiveHeapWriter::update_header_for_requested_obj(oop requested_obj, oop src_obj, Klass* src_klass) {
  	assert(UseCompressedClassPointers, "Archived heap only supported for compressed klasses");
  	narrowKlass nk = ArchiveBuilder::current()->get_requested_narrow_klass(src_klass);
  	address buffered_addr = requested_addr_to_buffered_addr(cast_from_oop<address>(requested_obj));
  	oop fake_oop = cast_to_oop(buffered_addr);
  	fake_oop->set_narrow_klass(nk);

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 :

  jdk/src/hotspot/share/oops/compressedOops.cpp

  Lines 195 to 231 in c3f10e8

  	void CompressedKlassPointers::initialize(address addr, size_t len) {
  	#ifdef _LP64
  	assert(is_valid_base(addr), "Address must be a valid encoding base");
  	address const end = addr + len;
  	address base;
  	int shift;
  	size_t range;
  	if (UseSharedSpaces || DumpSharedSpaces) {
  	// Special requirements if CDS is active:
  	// Encoding base and shift must be the same between dump and run time.
  	// CDS takes care that the SharedBaseAddress and CompressedClassSpaceSize
  	// are the same. Archive size will be probably different at runtime, but
  	// it can only be smaller than at, never larger, since archives get
  	// shrunk at the end of the dump process.
  	// From that it follows that the range [addr, len) we are handed in at
  	// runtime will start at the same address then at dumptime, and its len
  	// may be smaller at runtime then it was at dump time.
  	//
  	// To be very careful here, we avoid any optimizations and just keep using
  	// the same address and shift value. Specifically we avoid using zero-based
  	// encoding. We also set the expected value range to 4G (encoding range
  	// cannot be larger than that).
  	base = addr;
  	// JDK-8265705
  	// This is a temporary fix for aarch64: there, if the range-to-be-encoded is located
  	// below 32g, either encoding base should be zero or base should be aligned to 4G
  	// and shift should be zero. The simplest way to fix this for now is to force
  	// shift to zero for both runtime and dumptime.
  	// Note however that this is not a perfect solution. Ideally this whole function
  	// should be CDS agnostic, that would simplify it - and testing - a lot. See JDK-8267141
  	// for details.
  	shift = 0;

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. Thankfully, that minimal shift is zero in today's hotspot (but Lilliput may change that).

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. It introduces a new constant, ArchiveHeapWriter::precomputed_narrow_klass_shift, that holds the shift used to encode nKlass Ids (for the current hotspot, it is null). (A companion constant for the precomputed base makes little sense since the archive can be mapped everywhere; the only condition for using the precomputed narrow Klass Ids is to set the runtime base to the mapping start; I considered encoding this in a "delta" constant but Ioi convinced me otherwise).

I removed the "narrow_klass_" data holders from FileMap.

The only functional change is the fact that at dumptime, the JVM now is free to choose its base+shift for its own JVM. Encoding at runtime and for CDS=off does not change.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8311035: CDS should not use dump time JVM narrow Klass encoding to pre-compute Klass ids (Enhancement - P4)

Reviewers

• Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 7ed8cad0
• @ashu-mehra (no known openjdk.org user name / role)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14688/head:pull/14688
$ git checkout pull/14688

Update a local copy of the PR:
$ git checkout pull/14688
$ git pull https://git.openjdk.org/jdk.git pull/14688/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14688

View PR using the GUI difftool:
$ git pr show -t 14688

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14688.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@tstuefe The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 08: Full - Incremental (0286c510)
• 07: Full (0ea2fa10)
• 06: Full (7ed8cad0)
• 05: Full - Incremental (4dbfcc9e)
• 04: Full (4de3995d)
• 03: Full - Incremental (26a2b9d1)
• 02: Full (807412ea)
• 01: Full - Incremental (1e9bacd9)
• 00: Full (5211504b)

[iklam]

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:

jdk/src/hotspot/share/cds/metaspaceShared.cpp

Line 1149 in 46e4ee1

CompressedKlassPointers::initialize(cds_base, ccs_end - cds_base);

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

[tstuefe]

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:

jdk/src/hotspot/share/cds/metaspaceShared.cpp

Line 1149 in 46e4ee1

CompressedKlassPointers::initialize(cds_base, ccs_end - cds_base);

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

[openjdk]

@tstuefe this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout fix-cleanup-CDS-nKlass-encoding
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[tstuefe]

@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().

[tstuefe]

x86 test error unrelated.

[iklam]

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

[openjdk]

@tstuefe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 6 new commits pushed to the master branch:

• 6eba096: 8310999: Add @SInCE info in jdk.jsobject files
• 6ebb0e3: 8311023: assert(false) failed: EA: missing memory path
• 2cffef2: 8311290: Improve java.lang.ref.Cleaner rendered documentation
• 22e17c2: 8311180: Remove unused unneeded definitions from globalDefinitions
• cf82e31: 8311077: Fix -Wconversion warnings in jvmti code
• 00ac46c: 8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[tstuefe]

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

Thanks @iklam !

[tstuefe]

Could I have a second review, please? @ashu-mehra, maybe?

[ashu-mehra]

Is it possible for requested_base to ever be less than addr?

[tstuefe]

Not at the moment, no. Probably never. I separated those two mainly for cleanliness, since Klass range and encoding range are different things.

[ashu-mehra]

I am fine having them separately but that assert condition forced me to think if requested_base can ever be less than addr. If not, then I guess assert for requested_base == addr ?

[tstuefe]

Sure, makes sense.

[ashu-mehra]

Thanks for the changes. lgtm!

[tstuefe]

Thanks @iklam and @ashu-mehra !

/integrate

[openjdk]

Going to push as commit 0616648.
Since your change was applied there have been 6 commits pushed to the master branch:

• 6eba096: 8310999: Add @SInCE info in jdk.jsobject files
• 6ebb0e3: 8311023: assert(false) failed: EA: missing memory path
• 2cffef2: 8311290: Improve java.lang.ref.Cleaner rendered documentation
• 22e17c2: 8311180: Remove unused unneeded definitions from globalDefinitions
• cf82e31: 8311077: Fix -Wconversion warnings in jvmti code
• 00ac46c: 8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient

Your commit was automatically rebased without conflicts.

[openjdk]

@tstuefe Pushed as commit 0616648.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.