8357525: Default CDS archive becomes non-deterministic after JDK-8305895

[iklam]

Since JDK-8305895, interfaces and abstract classes are allocated outside of the class space. See here for the triggering condition

Theoretically such InstanceKlasses can have arbitrary addresses, so the "tie breaker test" of (uintptr_t(existing) < uintptr_t(klass)) in Klass::hash_insert() no longer produces a deterministic result across two JVM runs. As a result, we have seen several occurrence of non-deterministic CDS archives in our test pipeline for a short period after JDK-8305895 for pushed.

Thereafter, for some unknown reason, the problem disappears. However, we could just be lucky due to allocation policy changes in metaspace. The underlying cause still exists.

I wrote a proof of concept that shows the problem. See 3318570

(Without fix)

$ java -Xshare:dump -XX:+UseNewCode -XX:SharedArchiveFile=f1.jsa
$ java -Xshare:dump -XX:+UseNewCode2 -XX:SharedArchiveFile=f2.jsa
$ cksum f1.jsa f2.jsa
  2834014305 16105472 f1.jsa
  4126337207 16105472 f2.jsa

(With the fix in this PR)

$ java -Xshare:dump -XX:+UseNewCode -XX:+UseNewCode3 -XX:SharedArchiveFile=f1.jsa
$ java -Xshare:dump -XX:+UseNewCode2 -XX:+UseNewCode3 -XX:SharedArchiveFile=f2.jsa
$ cksum f1.jsa f2.jsa
  3637571299 16105472 f1.jsa
  3637571299 16105472 f2.jsa

With the fix, we re-hash the copy of the secondary_supers in the CDS archive. When InstanceKlasses are copied into the CDS archive, they are sorted by their names. This makes the (uintptr_t(existing) < uintptr_t(klass)) comparison produce deterministic results.

Unfortunately it's not possible to write a jtreg test for this problem.

Note: the "tie breaker test" was introduced in JDK-8180450. It adds an assumption that the address order of classes in the CDS archive buffer is the same as the classes in the class space, which can become invalid if the class space allocation algorithm changes.

---

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-8357525: Default CDS archive becomes non-deterministic after JDK-8305895 (Bug - P4)

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer)
• Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25373

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back iklam! 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]

@iklam 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:

8357525: Default CDS archive becomes non-deterministic after JDK-8305895

Reviewed-by: shade, coleenp

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 94 new commits pushed to the master branch:

• 4755276: 8357912: (fs) Remove @SInCE tag from java.nio.file.FileSystems.newFileSystem(Path,ClassLoader)
• da228e0: 8357145: CRC/Inflater/Deflater/Adler32 methods that take a ByteBuffer throw UOE if backed by shared memory segment
• d4b923d: 8357268: Use JavaNioAccess.getBufferAddress rather than DirectBuffer.address()
• ... and 91 more: https://git.openjdk.org/jdk/compare/4d7068923cd87fbfc2edee25406521b11580d153...master

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.

[openjdk]

@iklam 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

• 01: Full - Incremental (e632ac67)
• 00: Full (5532e378)

[shipilev]

This looks fine.

Looks to me we mis-attributing the root cause a bit: it is not just about JDK-8305895 that made some IK-s allocated at more arbitrary locations outside of Class Space. Even in Class Space there is a positional randomness, right?

So I think the actual root cause is the the address-based tie-breaker introduced in JDK-8180450. Let's link those issues up and change the synopsis accordingly?

[shipilev]

Suggested change

	// Note that the bitmap is guatanteed to be deterministic, regardless of the
	// Note that the bitmap is guaranteed to be deterministic, regardless of the

[iklam]

This looks fine.

Looks to me we mis-attributing the root cause a bit: it is not just about JDK-8305895 that made some IK-s allocated at more arbitrary locations outside of Class Space. Even in Class Space there is a positional randomness, right?

So I think the actual root cause is the the address-based tie-breaker introduced in JDK-8180450. Let's link those issues up and change the synopsis accordingly?

I updated the PR and JBS synopsis to include JDK-8180450.

Determinism is required only when creating the default CDS archive during the JDK build, where we run a single thread to load the classes. As far as I know, the address order in the class space happens to be the same as in the CDS output buffer, but that's just a happy co-incidence and could change in the future. This RFE will guard against such changes.

[shipilev]

I don't see the bug synopsis update, but PR looks good.

[coleenp]

It was actually JDK-8338526 this change that moved interface and abstract classes into non-class metaspace.

[coleenp]

This looks good but I also think the synopsis should be updated. A bit disconcerting that now remove_unshareable_info() has side effects. Maybe it should be renamed to something like make_shareable() with some future RFE.

[iklam]

Thanks @shipilev @coleenp for the review
/integrate

[openjdk]

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

• dede353: 8355798: Implement JEP 514: Ahead-of-Time Command Line Ergonomics
• b7f0f48: 8357681: Fixed the DigitList::toString method causing incorrect results during debugging
• bb2c80c: 8298420: Implement JEP 470: PEM Encodings of Cryptographic Objects (Preview)
• ... and 130 more: https://git.openjdk.org/jdk/compare/4d7068923cd87fbfc2edee25406521b11580d153...master

Your commit was automatically rebased without conflicts.

[openjdk]

@iklam Pushed as commit 2ec6ab3.

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