8256425: Obsolete Biased Locking in JDK 18 #4522

pchilano · 2021-06-17T15:37:40Z

Hi all,

Please review the following patch which handles the removal of biased locking code.

The third least significant bit of the markword is now always unused. I didn't try to give it back to the age field as it was prior to biased locking introduction since it will likely be taken away by other projects (probably Valhalla).

Regarding c1 changes, the scratch register passed to LIRGenerator::monitor_enter() was only used by biased locking code except in ppc, so in all other platforms I removed the scratch parameter from C1_MacroAssembler::lock_object() (except in s390 where it wasn't defined already).
We could probably just always use R0 as a temp register in lock_object() for ppc, since we were already using it as temp in biased_locking_enter(), and remove the scratch parameter from there too. Then we could remove the scratch field from LIR_OpLock. I haven't done that in this patch though.

For c2, type.hpp defined XorXNode, StoreXConditionalNode, LoadXNode and StoreXNode as needed by UseOptoBiasInlining. I see that LoadXNode and StoreXNode are also used by shenandoahSupport so I kept those two defines. I removed only the biased locking comments from the storeIConditional/storeLConditional implementations in .ad files since I don't know if they might be needed.

There are some tests that were only meaningful when run with biased locking enabled so I removed them.

Tested in mach5 tiers 1-7. I tested it builds also on ppc, s390 and arm32 but can't run any tests on those platforms so it would be good if somebody can do some sanity check on those ones.

Thanks,
Patricio

Progress

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

Issue

JDK-8256425: Obsolete Biased Locking in JDK 18

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to cba01d0
David Holmes (@dholmes-ora - Reviewer)
Daniel D. Daugherty (@dcubed-ojdk - Reviewer) ⚠️ Review applies to cb3b5e2
Richard Reingruber (@reinrich - Reviewer) ⚠️ Review applies to c601517

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4522

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

Using diff file

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

bridgekeeper · 2021-06-17T15:39:06Z

👋 Welcome back pchilanomate! 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-06-17T15:41:43Z

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

build
core-libs
hotspot
serviceability
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.

pchilano · 2021-06-17T15:47:29Z

/label remove build,core-libs, shenandoah,serviceability

openjdk · 2021-06-17T15:48:27Z

@pchilano
The build label was successfully removed.

The core-libs label was successfully removed.

The shenandoah label was successfully removed.

The serviceability label was successfully removed.

openjdk · 2021-06-17T15:49:48Z

@pchilano
The hotspot-runtime label was successfully added.

The hotspot-compiler label was successfully added.

openjdk · 2021-06-17T15:52:31Z

@pchilano
The hotspot-compiler label was successfully removed.

mlbridge · 2021-06-17T19:51:59Z

Webrevs

vnkozlov

Very nice clean up. Thank you. I have small nitpick and question about BiasedLocking flags deprecation. Obsolete flags table says: { "UseBiasedLocking", JDK_Version::jdk(15), JDK_Version::jdk(18), JDK_Version::jdk(19) },

It means in JDK 18 JVM have to accept flags on command line but issue warning.
May be I mistaking, but it means you can not remove flags declaration.
You can remove corresponding code.

vnkozlov · 2021-06-17T21:39:25Z

src/hotspot/cpu/ppc/vm_version_ppc.cpp

@@ -379,10 +379,6 @@ void VM_Version::initialize() {
  // Adjust RTM (Restricted Transactional Memory) flags.
  if (UseRTMLocking) {
    // If CPU or OS do not support TM:
-    // Can't continue because UseRTMLocking affects UseBiasedLocking flag


Can you fix in previous line TM -> RTM

mlbridge · 2021-06-18T03:18:59Z

Mailing list message from David Holmes on hotspot-runtime-dev:

On 18/06/2021 11:56 am, Vladimir Kozlov wrote:

Very nice clean up. Thank you. I have small nitpick and question about BiasedLocking flags deprecation. Obsolete flags [table](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/arguments.cpp#L537) says: ```{ "UseBiasedLocking", JDK_Version::jdk(15), JDK_Version::jdk(18), JDK_Version::jdk(19) },```

It means in JDK 18 JVM have to accept flags on command line but issue warning.

Correct.

May be I mistaking, but it means you can not remove flags declaration.

You can remove the flag (and must). Obsolete flags are handled purely by
lookup in the obsolete flag table. Once a flag is obsoleted there should
only be one occurrence left of it in the source code - in that table. :)

Cheers,
David
-----

mlbridge · 2021-06-18T03:19:02Z

Mailing list message from David Holmes on hotspot-runtime-dev:

On 18/06/2021 11:56 am, Vladimir Kozlov wrote:

Very nice clean up. Thank you. I have small nitpick and question about BiasedLocking flags deprecation. Obsolete flags [table](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/arguments.cpp#L537) says: ```{ "UseBiasedLocking", JDK_Version::jdk(15), JDK_Version::jdk(18), JDK_Version::jdk(19) },```

It means in JDK 18 JVM have to accept flags on command line but issue warning.

Correct.

May be I mistaking, but it means you can not remove flags declaration.

You can remove the flag (and must). Obsolete flags are handled purely by
lookup in the obsolete flag table. Once a flag is obsoleted there should
only be one occurrence left of it in the source code - in that table. :)

Cheers,
David
-----

reinrich · 2021-06-22T15:13:10Z

No issues from my own testing. Broader test coverage on all platforms is expected tomorrow.

Great, I'll wait for that. Thanks for all the testing Richard!

Tests look fine. Also on s390. Unfortunately on ppc64le the tests didn't succeed because of another change. I'd suggest to wait one more day if you don't mind.

Richard.

pchilano · 2021-06-22T15:49:53Z

Hi Richard,

Tests look fine. Also on s390. Unfortunately on ppc64le the tests didn't succeed because of another change. I'd suggest to wait one more day if you don't mind.

No problem, let me know when tests complete successfully.

Thanks again!

Patricio

reinrich · 2021-06-23T17:00:16Z

Hi Patricio,

ppc64le test results are available now. There's no failure related to this change.

Thanks for your patience,
Richard.

reinrich

Hi Patricio,

as stated before I've reviewed the part of this change that is related to JDK-8227745 and found it to be good.

Good thing to get rid of so much complex code!

Thanks, Richard.

openjdk · 2021-06-23T17:09:28Z

@pchilano 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 8256425
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

pchilano · 2021-06-23T17:38:50Z

Hi Patricio,

as stated before I've reviewed the part of this change that is related to JDK-8227745 and found it to be good.

Good thing to get rid of so much complex code!

Great, thanks for reviewing and all the testing Richard!

Patricio

pchilano · 2021-06-24T18:53:15Z

Thanks all for reviews and comments!

pchilano · 2021-06-24T18:53:27Z

/integrate

openjdk · 2021-06-24T18:54:49Z

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

595446b: 8269087: CheckSegmentedCodeCache test fails in an emulated-client VM
7c31903: 8267075: jcmd VM.cds should print directory of the output files
e515873: 8269216: Useless initialization in com/sun/crypto/provider/PBES2Parameters.java
51d9159: 8236212: CompiledMethodLoad and CompiledMethodUnload events can be posted in START phase
280f2d5: 8268433: serviceability/dcmd/framework/VMVersionTest.java fails with Unable to send object throw not established PipeIO Listener Thread connection
f375916: 8269186: [REDO] Remove CodeCache::mark_for_evol_deoptimization() method

Your commit was automatically rebased without conflicts.

openjdk · 2021-06-24T18:55:06Z

@pchilano Pushed as commit 2fd7943.

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

dougxc · 2023-05-10T11:24:37Z

This change caused a performance regression for GraalVM in that it effectively disabled the intrinsic for System.identityHashCode.
In future, if you make changes to JVMCI files, please ensure I or @tkrodriguez are notified of the PR.

@vnkozlov I know you've been pretty good at alerting us to such PRs but I wonder if there's a more automated way to achieve this?

stefank · 2023-05-10T11:41:31Z

I know you've been pretty good at alerting us to such PRs but I wonder if there's a more automated way to achieve this?

We have a mapping between directories and mailing lists that should be notified when changes are done to files in those directories. See:
https://github.com/openjdk/skara/blob/0043fdf3aab5a5dee4ad6e35915bb7962b39ce2e/config/mailinglist/rules/jdk.json#L250

Here you can see that the hotspot-compiler mailing list gets notified if changes are made to "src/hotspot/share/jvmci/". My guess is that it would be possible to edit this file to send mail to the Graal OpenJDK mailing lists.

TobiHartmann · 2023-05-10T11:42:05Z

See also https://bugs.openjdk.org/browse/SKARA-1703.

dougxc · 2023-05-10T12:05:56Z

Thanks - I've opened https://bugs.openjdk.org/browse/SKARA-1905.

pchilano added 2 commits June 17, 2021 01:27

8256425: Obsolete Biased Locking in JDK 18

1162261

Update java manpage

cba01d0

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Jun 17, 2021

openjdk bot removed build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org serviceability serviceability-dev@openjdk.org labels Jun 17, 2021

openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jun 17, 2021

openjdk bot removed the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 17, 2021

pchilano closed this Jun 17, 2021

pchilano changed the title ~~8256425: Obsolete Biased Locking in JDK 18~~ . Jun 17, 2021

pchilano changed the title . 8256425: Obsolete Biased Locking in JDK 18 Jun 17, 2021

pchilano reopened this Jun 17, 2021

pchilano marked this pull request as ready for review June 17, 2021 19:45

openjdk bot added the rfr Pull request is ready for review label Jun 17, 2021

vnkozlov reviewed Jun 18, 2021

View reviewed changes

Un-ProblemList serviceability tests (8268574 and 8268644)

c601517

reinrich approved these changes Jun 23, 2021

View reviewed changes

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jun 23, 2021

pchilano added 2 commits June 23, 2021 14:45

Merge master

dc06c35

remove extra whitespace

8d10c0e

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels Jun 23, 2021

fix cast in added whitebox method after 8268368

a1164af

dholmes-ora approved these changes Jun 24, 2021

View reviewed changes

openjdk bot closed this Jun 24, 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 Jun 24, 2021

pchilano deleted the 8256425 branch July 19, 2021 04:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8256425: Obsolete Biased Locking in JDK 18 #4522

8256425: Obsolete Biased Locking in JDK 18 #4522

pchilano commented Jun 17, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

pchilano commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

mlbridge bot commented Jun 17, 2021 •

edited

Loading

vnkozlov left a comment

vnkozlov Jun 17, 2021

pchilano Jun 18, 2021

mlbridge bot commented Jun 18, 2021

mlbridge bot commented Jun 18, 2021

reinrich commented Jun 22, 2021

pchilano commented Jun 22, 2021

reinrich commented Jun 23, 2021

reinrich left a comment

openjdk bot commented Jun 23, 2021

pchilano commented Jun 23, 2021

pchilano commented Jun 24, 2021

pchilano commented Jun 24, 2021

openjdk bot commented Jun 24, 2021

openjdk bot commented Jun 24, 2021

dougxc commented May 10, 2023

stefank commented May 10, 2023

TobiHartmann commented May 10, 2023

dougxc commented May 10, 2023

8256425: Obsolete Biased Locking in JDK 18 #4522

8256425: Obsolete Biased Locking in JDK 18 #4522

Conversation

pchilano commented Jun 17, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

pchilano commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

mlbridge bot commented Jun 17, 2021 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov Jun 17, 2021

Choose a reason for hiding this comment

pchilano Jun 18, 2021

Choose a reason for hiding this comment

mlbridge bot commented Jun 18, 2021

mlbridge bot commented Jun 18, 2021

reinrich commented Jun 22, 2021

pchilano commented Jun 22, 2021

reinrich commented Jun 23, 2021

reinrich left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 23, 2021

pchilano commented Jun 23, 2021

pchilano commented Jun 24, 2021

pchilano commented Jun 24, 2021

openjdk bot commented Jun 24, 2021

openjdk bot commented Jun 24, 2021

dougxc commented May 10, 2023

stefank commented May 10, 2023

TobiHartmann commented May 10, 2023

dougxc commented May 10, 2023

pchilano commented Jun 17, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 17, 2021 •

edited

Loading