8317440: Lock rank checking fails when code root set is modified with the Servicelock held after JDK-8315503

[tschatzl]

Hi all,

please review this change that fixes lock ranking after recent changes to the code root set, now using a CHT.

The issue came up because the lock rank of the CHT lock has been larger than the rank of the Servicethread_lock where it is possible that code roots can be added.

The suggested solution is to fix up the lock rankings to work; actually this PR contains two variants:

1. one that statically sets the lock ranks of the CHT lock (and the ThreadSMR_lock that can be used during CHT operation) to something smaller than Servicethread_lock.
2. one that allows setting of the CHT lock rank via parameter as well (the last commit changed the code to variant 1).

The other lock ranking changes to Metaspace_lock and ContinuationRelativize_lock are simply undos of the respective changes in JDK-8315503.

Testing: tier1-8 for variant 2), tier 1-7 for variant 1)

Thanks,
Thomas

---

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-8317440: Lock rank checking fails when code root set is modified with the Servicelock held after JDK-8315503 (Bug - P2)

Reviewers

• Coleen Phillimore (@coleenp - Reviewer)
• Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16062

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

• 00: Full (ed6a5e6c)

[tschatzl]

/label add hotspot-gc

[openjdk]

@tschatzl
The hotspot-gc label was successfully added.

[coleenp]

Variant 1 seems ok. Uses of the CHT shouldn't take locks, so having a low lock ranking for CHT lock seems like it'll be fine (I can't find where it takes the ThreadsSMRDelete_lock). If any of this breaks, we can try approach #2 next.

[openjdk]

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

8317440: Lock rank checking fails when code root set is modified with the Servicelock held after JDK-8315503

Reviewed-by: coleenp, ayang

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

• b3cc0c8: 8317318: Serial: Change GenCollectedHeap to SerialHeap in whitebox
• 691db5d: 8317592: Serial: Remove Space::toContiguousSpace
• ec9ba5d: 8317660: [BACKOUT] 8269393: store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64
• 7162624: 8269393: store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64
• f0d66d1: 8317502: Add asserts to check for non-null in ciInstance::java_lang_Class_klass
• 991ce84: 4964430: (spec) missing IllegalStateException exception requirement for javax.crypto.Cipher.doFinal
• 8a30c2a: 8317443: StackOverflowError on calling ListFormat::getInstance() for Norwegian locales
• a1c9587: 8313348: Fix typo in JFormattedTextField: 'it self'
• a8eacb3: 8317240: Promptly free OopMapEntry after fail to insert the entry to OopMapCache
• 4c5b66d: 8317522: Test logic for BODY_CF in AbstractThrowingSubscribers.java is wrong
• ... and 16 more: https://git.openjdk.org/jdk/compare/0b0f8b55a6becff269ecf7aa19db12e998e238cd...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.

[tschatzl]

Variant 1 seems ok. Uses of the CHT shouldn't take locks, so having a low lock ranking for CHT lock seems like it'll be fine (I can't find where it takes the ThreadsSMRDelete_lock). If any of this breaks, we can try approach #2 next.

Thread lists synchronization in GlobalCounter::write_synchronize() uses the ThreadsSMRDelete_lock via JavaThreadIteratorWithHandle->ThreadsListHandle->SafeThreadsListPtr in the destructor ...

[tschatzl]

Thanks @coleenp @albertnetymk for your reviews
/integrate

[openjdk]

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

• bcc986b: 8317601: Windows build on WSL broken after JDK-8317340
• 460ebcd: 8314978: Multiple server call from connection failing with expect100 in getOutputStream
• dc4bc4f: 8306819: Consider disabling the compiler's default active annotation processing
• a4e9168: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays)
• 6c6beba: 8317128: java/nio/file/Files/CopyAndMove.java failed with AccessDeniedException
• b62e774: 8317515: Unify the code of the parse*() families of methods in j.l.Integer and j.l.Long
• a64794b: 8317560: Change to Xcode 14.3.1 for building on macOS at Oracle
• b3cc0c8: 8317318: Serial: Change GenCollectedHeap to SerialHeap in whitebox
• 691db5d: 8317592: Serial: Remove Space::toContiguousSpace
• ec9ba5d: 8317660: [BACKOUT] 8269393: store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64
• ... and 23 more: https://git.openjdk.org/jdk/compare/0b0f8b55a6becff269ecf7aa19db12e998e238cd...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tschatzl Pushed as commit 0cf1a55.

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

[coleenp]

Going with variant 2.

[tschatzl]

Going with variant 2.

So a new lock rank issue has been found? sigh