8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT #16603

xmas92 · 2023-11-10T12:18:29Z

LM_LIGHTWEIGHT only uses the lock bits for its locking. This leaves the hashCode bits free when a monitor is not inflated. So instead of inflating when installing the hashCode on a fast locked object it can simply use the hashCode bits in the markWord.

The mark word transitions Unlocked (0b01) <=> Locked (0b00) are done by retrying the CAS if it fails due to non-lock bit changes.
The mark word transitions Monitor (0b10) <=> Locked/Unlocked (0b0X) are the same as before, inflation already handles hash codes. This change does not interact with the mark word if it is in a Monitor (0b10) state, so the strong CAS which is used for deflation are still valid, and will not fail to any other reason than the cooperative race to help transition the mark word during deflation.

This is dependent on JDK-8319778 simply because JDK-8319797 is dependent on both this and JDK-8319778.

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-8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT (Enhancement - P4)

Reviewers

Roman Kennke (@rkennke - Reviewer) ⚠️ Review applies to 46d4c1b1
Daniel D. Daugherty (@dcubed-ojdk - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to fdbfbf8a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16603

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

Using diff file

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

Webrev

Link to Webrev Comment

…IGHTWEIGHT

xmas92 · 2023-11-10T12:18:37Z

Testing

linux-x64, linux-x64-debug
- LM_LEGACY
  - tier 1-7
- LM_LIGHTWEIGHT
  - tier 1-7
linux-aarch64, linux-aarch64-debug,macosx-aarch64, macosx-aarch64-debug,macosx-x64, macosx-x64-debug, windows-x64, windows-x64-debug
- LM_LEGACY
  - tier 1-6
- LM_LIGHTWEIGHT
  - tier 1-6
GitHub actions

bridgekeeper · 2023-11-10T12:19:32Z

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

openjdk · 2023-11-10T12:20:33Z

@xmas92 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 · 2023-11-10T12:23:17Z

Webrevs

rkennke

This is great! I've done something like that in my original LW-locking PR, but then ripped it out to keep it simpler. Never got around to re-do it. Changes look good to me!

dholmes-ora

Seems reasonable - though more complex than I had envisaged. One query below and a change requested for the test.

Thanks

src/hotspot/share/runtime/synchronizer.cpp

test/hotspot/jtreg/runtime/whitebox/TestWBDeflateIdleMonitors.java

dholmes-ora

Noting further from me. Runtime changes look good. Best to get a compiler dev to okay the C2 changes.

Thanks

src/hotspot/share/runtime/synchronizer.cpp

test/hotspot/jtreg/runtime/whitebox/TestWBDeflateIdleMonitors.java

dcubed-ojdk · 2023-11-16T19:15:06Z

src/hotspot/cpu/x86/sharedRuntime_x86.cpp

+  if (LockingMode == LM_LIGHTWEIGHT) {
+    // check if monitor
+    __ testptr(result, markWord::monitor_value);
+    __ jcc(Assembler::notZero, slowCase);
+  } else {


Not needed for other platforms? Or will that be done with other bugs or sub-tasks?

I only found that x86 and 32bit arm does this in the shared runtime. I can create a jbs entry for arm. If it is the case that other platform do this for C1, then I cannot find where.

dcubed-ojdk

Thumbs up. I have a couple of editorial comments, but I think that's it.

I did a review of the C2 changes and I don't see anything obviously wrong,
but having a C2 reviewer would be very, very useful.

This needs additional testing. I recommend Tier{1,2,3} on all the usual Oracle
platforms. Tier{4,5,6} would catch the uses of hash code stuff in more stress
related configs by the Serviceability tests.

TobiHartmann

Compiler changes look good and in-line with the runtime changes.

src/hotspot/share/opto/library_call.cpp

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

openjdk-notifier · 2023-11-27T07:25:28Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout JDK-8319773
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2023-11-27T07:26:49Z

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

8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT

Reviewed-by: rkennke, dcubed, thartmann

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 · 2023-11-27T07:26:51Z

⚠️ @xmas92 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

dcubed-ojdk

I've re-reviewed the changes from v03 -> v08.

Thumbs up with what you currently have. There's still one open query
in FastHashCode about whether the VMThread can get in there
during JVM/TI tagging... It may be best to file a follow up bug for
chasing down that detail and possibly addressing it... Or you could
address it with one more change in this PR...

src/hotspot/share/runtime/synchronizer.cpp

bridgekeeper · 2024-01-04T17:32:37Z

@xmas92 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

src/hotspot/share/runtime/synchronizer.cpp

dcubed-ojdk · 2024-01-09T22:33:02Z

The last merge was on 2023.12.04 so I'll review again after this PR is merged
with newer jdk/jdk baseline bits.

xmas92 · 2024-01-12T14:33:45Z

Ran tier1-5 all Oracle platforms with both LM_LEGACY and LM_LIGHTWEIGHT after the merge.
Will go ahead and integrate this. I'll create an RFE for the arm32 hashcode optimisation as well as for the re-inflate race.

xmas92 · 2024-01-12T14:33:49Z

/integrate

openjdk · 2024-01-12T14:34:44Z

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

e22ab10: 8322537: Parallel: Remove experimental adjustment in PSAdaptiveSizePolicy
be900f1: 8323425: JFR: Auto-generated filename doesn't work with time-limited recording
68c4286: 8323008: filter out harmful -std* flags added by autoconf from CXX
7dc9dd6: 8234502: Merge GenCollectedHeap and SerialHeap
ed18222: 8323190: Segfault during deoptimization of C2-compiled code
3e19bf8: 8323529: Relativize test image dependencies in microbenchmarks
ba23025: 8322957: Generational ZGC: Relocation selection must join the STS
7c3a39f: 8323297: Fix incorrect placement of precompiled.hpp include lines
e72723d: 8323296: java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java#id1 timed out
66520be: 8280056: gtest/LargePageGtests.java#use-large-pages failed "os.release_one_mapping_multi_commits_vm"
... and 31 more: https://git.openjdk.org/jdk/compare/f0169341846360d202c973ec368fdc3c5bf1ae5d...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-12T14:34:50Z

@xmas92 Pushed as commit 65a0672.

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

xmas92 added 2 commits November 9, 2023 13:30

8319778: Remove unreachable code in ObjectSynchronizer::exit

90676ce

8319773: Avoid inflating monitors when installing hash codes for LM_L…

46d4c1b

…IGHTWEIGHT

openjdk bot added the rfr Pull request is ready for review label Nov 10, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 10, 2023

rkennke approved these changes Nov 10, 2023

View reviewed changes

openjdk bot mentioned this pull request Nov 10, 2023

8319797: Recursive lightweight locking: Runtime implementation #16606

Closed

3 tasks

dholmes-ora suggested changes Nov 13, 2023

View reviewed changes

src/hotspot/share/runtime/synchronizer.cpp Show resolved Hide resolved

test/hotspot/jtreg/runtime/whitebox/TestWBDeflateIdleMonitors.java Outdated Show resolved Hide resolved

Simplify test.

f10571e

dholmes-ora reviewed Nov 13, 2023

View reviewed changes

xmas92 added 2 commits November 14, 2023 11:14

Merge remote-tracking branch 'upstream_jdk/master' into JDK-8319778

bb1a203

Merge remote-tracking branch 'upstream_jdk/pr/16602' into JDK-8319773

e605568

coleenp reviewed Nov 15, 2023

View reviewed changes

src/hotspot/share/runtime/synchronizer.cpp Show resolved Hide resolved

Move is_lock_owned closer to its only use

eac6d69

dcubed-ojdk reviewed Nov 16, 2023

View reviewed changes

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

dcubed-ojdk reviewed Nov 16, 2023

View reviewed changes

src/hotspot/share/runtime/synchronizer.cpp Show resolved Hide resolved

dcubed-ojdk reviewed Nov 16, 2023

View reviewed changes

test/hotspot/jtreg/runtime/whitebox/TestWBDeflateIdleMonitors.java Show resolved Hide resolved

dcubed-ojdk reviewed Nov 16, 2023

View reviewed changes

dcubed-ojdk approved these changes Nov 16, 2023

View reviewed changes

xmas92 added 5 commits November 17, 2023 08:23

Use more familiar CAS variable names and pattern

6fbdc68

Merge remote-tracking branch 'upstream_jdk/master' into JDK-8319778

4cf7b70

Merge remote-tracking branch 'upstream_jdk/pr/16602' into JDK-8319773

a9b39f7

Use is_neutral over is_unlocked

6bd1118

Add retry CAS comment

fdbfbf8

TobiHartmann approved these changes Nov 20, 2023

View reviewed changes

src/hotspot/share/opto/library_call.cpp Outdated Show resolved Hide resolved

xmas92 and others added 2 commits November 21, 2023 14:54

Update src/hotspot/share/opto/library_call.cpp

560dd15

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

Fix copy paste typo.

b9cc13a

xmas92 added 2 commits November 21, 2023 14:27

Merge remote-tracking branch 'upstream_jdk/master' into JDK-8319778

add233f

Merge remote-tracking branch 'upstream_jdk/pr/16602' into JDK-8319773

b406141

openjdk-notifier bot changed the base branch from pr/16602 to master November 27, 2023 07:25

openjdk bot added the ready Pull request is ready to be integrated label Nov 27, 2023

dcubed-ojdk approved these changes Nov 29, 2023

View reviewed changes

src/hotspot/share/runtime/synchronizer.cpp Show resolved Hide resolved

xmas92 added 2 commits November 30, 2023 07:33

Merge remote-tracking branch 'upstream_jdk/master' into JDK-8319773

4508ef5

Merge remote-tracking branch 'upstream_jdk/master' into JDK-8319773

1b907f9

dcubed-ojdk approved these changes Jan 9, 2024

View reviewed changes

src/hotspot/share/runtime/synchronizer.cpp Show resolved Hide resolved

Merge remote-tracking branch 'upstream_jdk/master' into JDK-8319773

a83ad37

openjdk bot added the integrated Pull request has been integrated label Jan 12, 2024

openjdk bot closed this Jan 12, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT #16603

8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT #16603

xmas92 commented Nov 10, 2023 •

edited by openjdk bot

Loading

xmas92 commented Nov 10, 2023 •

edited

Loading

bridgekeeper bot commented Nov 10, 2023

openjdk bot commented Nov 10, 2023

mlbridge bot commented Nov 10, 2023 •

edited

Loading

rkennke left a comment

dholmes-ora left a comment

dholmes-ora left a comment

dcubed-ojdk Nov 16, 2023

xmas92 Nov 17, 2023

dcubed-ojdk left a comment

TobiHartmann left a comment

openjdk-notifier bot commented Nov 27, 2023

openjdk bot commented Nov 27, 2023 •

edited

Loading

openjdk bot commented Nov 27, 2023

dcubed-ojdk left a comment

bridgekeeper bot commented Jan 4, 2024

dcubed-ojdk commented Jan 9, 2024

xmas92 commented Jan 12, 2024

xmas92 commented Jan 12, 2024

openjdk bot commented Jan 12, 2024

openjdk bot commented Jan 12, 2024

8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT #16603

8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT #16603

Conversation

xmas92 commented Nov 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

xmas92 commented Nov 10, 2023 • edited Loading

Testing

bridgekeeper bot commented Nov 10, 2023

openjdk bot commented Nov 10, 2023

mlbridge bot commented Nov 10, 2023 • edited Loading

Webrevs

rkennke left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dcubed-ojdk Nov 16, 2023

Choose a reason for hiding this comment

xmas92 Nov 17, 2023

Choose a reason for hiding this comment

dcubed-ojdk left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

openjdk-notifier bot commented Nov 27, 2023

openjdk bot commented Nov 27, 2023 • edited Loading

openjdk bot commented Nov 27, 2023

dcubed-ojdk left a comment

Choose a reason for hiding this comment

bridgekeeper bot commented Jan 4, 2024

dcubed-ojdk commented Jan 9, 2024

xmas92 commented Jan 12, 2024

xmas92 commented Jan 12, 2024

openjdk bot commented Jan 12, 2024

openjdk bot commented Jan 12, 2024

xmas92 commented Nov 10, 2023 •

edited by openjdk bot

Loading

xmas92 commented Nov 10, 2023 •

edited

Loading

mlbridge bot commented Nov 10, 2023 •

edited

Loading

openjdk bot commented Nov 27, 2023 •

edited

Loading