8273917: Remove 'leaf' ranking for Mutex #5801

coleenp · 2021-10-03T21:08:51Z

This change removes 'leaf' ranking. The previous change for JDK-8273915 divided the 'leaf' ranked locks that didn't safepoint check into the rank 'nosafepoint', so all the 'leaf' ranking locks left were safepoint_check_always.

The rank 'nonleaf' (to be renamed 'safepoint' in the next change) is the top mutex rank.

The transformation in this change is as follows:
nonleaf+n => nonleaf - Generally these 'nonleaf' mutex were top level locks)
leaf => nonleaf - Many of these locks were top level locks
leaf => nonleaf-2 - Assuming that they were 'leaf' and 2 levels less than some existing nonleaf lock
leaf-n => nonleaf-n

The new mutex rankings reflect their rankings based on my logging, except for a couple shenandoah locks which I didn't observe, so I made them nonleaf-2.

This change also introduces a relative mutex ranking macro, so that a Mutex/Monitor can be defined in terms of a mutex that it holds while trying to acquire it. So these relative mutex are moved to the end of the init function in mutexLocker.cpp.

This has been tested with tier1-8, and retesting tier1-3 locally in progress.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8273917: Remove 'leaf' ranking for Mutex

Reviewers

Erik Österlund (@fisk - Reviewer) ⚠️ Review applies to bc4022c
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5801/head:pull/5801
$ git checkout pull/5801

Update a local copy of the PR:
$ git checkout pull/5801
$ git pull https://git.openjdk.java.net/jdk pull/5801/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5801

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5801.diff

coleenp · 2021-10-03T22:47:09Z

src/hotspot/share/oops/methodData.cpp

@@ -1206,7 +1206,8 @@ void MethodData::post_initialize(BytecodeStream* stream) {
 // Initialize the MethodData* corresponding to a given method.
 MethodData::MethodData(const methodHandle& method)
  : _method(method()),
-    _extra_data_lock(Mutex::leaf, "MDO extra data lock", Mutex::_safepoint_check_always),
+    // Holds Compile_lock
+    _extra_data_lock(Mutex::nonleaf-2, "MDOExtraData_lock", Mutex::_safepoint_check_always),


MDO lock (nonleaf-2) is taken when the Compile_lock is held (nonleaf-1) which is taken while the MethodCompileQueue_lock (nonleaf) is held.

bridgekeeper · 2021-10-04T06:34:16Z

👋 Welcome back coleenp! 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 · 2021-10-04T06:48:37Z

@coleenp The following labels will be automatically applied to this pull request:

hotspot
shenandoah

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

mlbridge · 2021-10-04T06:51:49Z

Webrevs

coleenp · 2021-10-05T13:25:43Z

According to my logging, the shenandoah locks don't have any dependencies when running runThese with shenandoah, but could someone confirm this is ok to make them nonleaf? @zhengyu123 ?
$ grep -r ShenandoahAllocFailureGC_lock
38_ShenandoahAllocFailureGC_lock:def(ShenandoahAllocFailureGC_lock , PaddedMonitor, nonleaf, _safepoint_check_always, true);
$ grep -r ShenandoahRequestedGC_lock
38_ShenandoahRequestedGC_lock:def(ShenandoahRequestedGC_lock , PaddedMonitor, nonleaf, _safepoint_check_always, true);

_wait_monitor (should rename to a consistent lock name) depends on PeriodicTask_lock:
$ grep -r PeriodicTask_lock
38_PeriodicTask_lock:def(PeriodicTask_lock , PaddedMonitor, nonleaf, _safepoint_check_always, true);
37__wait_monitor:def(_wait_monitor , PaddedMonitor, PeriodicTask_lock, _safepoint_check_always, true);

fisk

I think this is a step in the right direction, and I heard great things will come in the next patch. Thanks for reducing the changes to nonleaf - 2. Go Coleen, go! Also looks good.

openjdk · 2021-10-05T16:59:33Z

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

8273917: Remove 'leaf' ranking for Mutex

Reviewed-by: eosterlund, dholmes

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

986ee5d: 8274670: Improve version string handling in SA
df7b0c7: 8274715: Implement forEach in Collections.CopiesList
d4e8712: 8274797: ProblemList resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java on macosx-x64
4726267: 8274642: jdk/jshell/CommandCompletionTest.java fails with NoSuchElementException after JDK-8271287
83b2219: 8273612: Fix for JDK-8272873 causes timeout in running some tests with -Xcomp
d34ec6c: 8274793: Suppress warnings on non-serializable non-transient instance fields in sun.net
332f067: 8274729: Define Position.NOPOS == Diagnostic.NOPOS
1e75203: 8274656: Remove default_checksum and safe_checksum_type from krb5.conf
03d3c03: 8273670: Remove weak etypes from default krb5 etype list
c391e59: 8274244: ReportOnImportedModuleAnnotation.java fails on rerun
... and 30 more: https://git.openjdk.java.net/jdk/compare/9ca6bf0d22ac39d4ee29c756903038f09087ca12...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.

coleenp · 2021-10-05T19:46:11Z

Thank you Erik!

dholmes-ora

Hi Coleen,

A minor comment below but otherwise this all seems okay.

Thanks,
David

src/hotspot/share/runtime/mutex.cpp

coleenp · 2021-10-06T01:18:52Z

Thank you for reviewing, David.

dholmes-ora

Thanks for that tweak.

David

coleenp · 2021-10-06T12:13:24Z

Thanks for the reviews Erik and David.
/integrate

openjdk · 2021-10-06T12:14:41Z

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

c80a612: 8273381: Assert in PtrQueueBufferAllocatorTest.stress_free_list_allocator_vm
9759fcb: 8274496: Use String.contains() instead of String.indexOf() in java.desktop
cdf8930: 8274625: Search field placeholder behavior
df125f6: 8273410: IR verification framework fails with "Should find method name in validIrRulesMap"
c74726d: 8274730: AArch64: AES/GCM acceleration is broken by the fix for JDK-8273297
2faced0: 8274738: ZGC: Use relaxed atomic load when reading bits in the live map
8a4d2b4: 8274680: Remove unnecessary conversion to String in java.desktop
986ee5d: 8274670: Improve version string handling in SA
df7b0c7: 8274715: Implement forEach in Collections.CopiesList
d4e8712: 8274797: ProblemList resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java on macosx-x64
... and 37 more: https://git.openjdk.java.net/jdk/compare/9ca6bf0d22ac39d4ee29c756903038f09087ca12...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-06T12:14:53Z

@coleenp Pushed as commit b8af6a9.

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

coleenp added 2 commits October 3, 2021 16:38

8273917: Remove 'leaf' ranking for Mutex

2ca4bd7

Fix lock name.

34c43ec

coleenp commented Oct 3, 2021

View reviewed changes

coleenp marked this pull request as ready for review October 3, 2021 22:55

openjdk bot added the rfr Pull request is ready for review label Oct 4, 2021

openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Oct 4, 2021

Fix Minimal build.

4596df6

Adjust some nonleaf-2 ranks.

bc4022c

fisk approved these changes Oct 5, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 5, 2021

dholmes-ora approved these changes Oct 6, 2021

View reviewed changes

src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved

Combine bad rank range checks.

bd6ff18

dholmes-ora approved these changes Oct 6, 2021

View reviewed changes

openjdk bot closed this Oct 6, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 6, 2021

coleenp deleted the remove-leaf branch October 6, 2021 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8273917: Remove 'leaf' ranking for Mutex #5801

8273917: Remove 'leaf' ranking for Mutex #5801

coleenp commented Oct 3, 2021 •

edited by openjdk bot

Loading

coleenp Oct 3, 2021

bridgekeeper bot commented Oct 4, 2021

openjdk bot commented Oct 4, 2021

mlbridge bot commented Oct 4, 2021 •

edited

Loading

coleenp commented Oct 5, 2021 •

edited

Loading

fisk left a comment

openjdk bot commented Oct 5, 2021 •

edited

Loading

coleenp commented Oct 5, 2021

dholmes-ora left a comment

coleenp commented Oct 6, 2021

dholmes-ora left a comment

coleenp commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

8273917: Remove 'leaf' ranking for Mutex #5801

8273917: Remove 'leaf' ranking for Mutex #5801

Conversation

coleenp commented Oct 3, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

coleenp Oct 3, 2021

Choose a reason for hiding this comment

bridgekeeper bot commented Oct 4, 2021

openjdk bot commented Oct 4, 2021

mlbridge bot commented Oct 4, 2021 • edited Loading

Webrevs

coleenp commented Oct 5, 2021 • edited Loading

fisk left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 5, 2021 • edited Loading

coleenp commented Oct 5, 2021

dholmes-ora left a comment

Choose a reason for hiding this comment

coleenp commented Oct 6, 2021

dholmes-ora left a comment

Choose a reason for hiding this comment

coleenp commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

coleenp commented Oct 3, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 4, 2021 •

edited

Loading

coleenp commented Oct 5, 2021 •

edited

Loading

openjdk bot commented Oct 5, 2021 •

edited

Loading