8367982: Unify ObjectSynchronizer and LightweightSynchronizer #27915

fbredber · 2025-10-21T13:11:45Z

This is the last PR in a series of PRs (see: JDK-8344261) to obsolete the LockingMode flag and related code.

The main focus is to to unify ObjectSynchronizer and LightweightSynchronizer.
There used to be a number of "dispatch functions" to redirect calls depending on the setting of the LockingMode flag.
Since we now only have lightweight locking, there is no longer any need for those dispatch functions, so I removed them.
To remove the dispatch functions I renamed the corresponding lightweight functions and call them directly.
This ultimately led me to remove "lightweight" from the function names and go back to "fast" instead, just to avoid having some with, and some without the "lightweight" part of the name.

This PR also include a small simplification of ObjectSynchronizer::FastHashCode.

Tested tier1-7 (on supported platforms) without seeing any problems that can be traced to this code change.
All other platforms (arm, ppc, riscv, s390) has been sanity checked using QEMU.

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-8367982: Unify ObjectSynchronizer and LightweightSynchronizer (Sub-task - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27915

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-10-21T13:13:41Z

👋 Welcome back fbredberg! 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 · 2025-10-21T13:14:11Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-10-21T13:14:58Z

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

hotspot
serviceability

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 · 2025-10-23T08:37:34Z

Webrevs

01: Full - Incremental (6e331721)
00: Full (993d6137)

fbredber · 2025-10-23T08:38:05Z

@bulasevich, @TheRealMDoerr, @RealFYang, @offamitkumar
Hi guys!
This is the last PR in a series of PRs to obsolete the LockingMode flag and related code.
So please please grab a copy, run it on your favorite platform, and tell me if there's anything wrong with it.
Thanks in advance.

TheRealMDoerr · 2025-10-23T12:06:28Z

Thanks for cleaning this up on all platforms! Works on PPC64.
Indentation could be improved at many places where the next line was aligned (all platforms and shared code).

pchilano

Thanks for doing this cleanup. I only found some instances in comments where lightweight referred to the locking mode and not just the fast path, so the word should be removed rather than replaced with fast.

src/hotspot/share/oops/markWord.hpp

src/hotspot/share/runtime/objectMonitor.hpp

src/hotspot/share/runtime/objectMonitor.inline.hpp

pchilano · 2025-10-23T17:29:29Z

src/hotspot/share/runtime/synchronizer.cpp

      // stability and then install the hash.
-    } else if (mark.has_monitor()) {
+    } else {
+      assert(!mark.is_unlocked() && !mark.is_fast_locked(), "invariant");


Note that mark.monitor() below already asserts mark.has_monitor() which is stronger.

Good point, but I still like to keep the assert() on 648 for clarity. Would you rather see it removed?

RealFYang · 2025-10-24T01:16:55Z

@bulasevich, @TheRealMDoerr, @RealFYang, @offamitkumar Hi guys! This is the last PR in a series of PRs to obsolete the LockingMode flag and related code. So please please grab a copy, run it on your favorite platform, and tell me if there's anything wrong with it. Thanks in advance.

Hello, A simple tier1 test is good on linux-riscv64. I checked the riscv part and I only see in indentation issues as pointed out by @TheRealMDoerr.

offamitkumar · 2025-10-24T07:39:56Z

@bulasevich, @TheRealMDoerr, @RealFYang, @offamitkumar Hi guys! This is the last PR in a series of PRs to obsolete the LockingMode flag and related code. So please please grab a copy, run it on your favorite platform, and tell me if there's anything wrong with it. Thanks in advance.

Hi, I ran tier1 and seems like on s390 everything is good.

coleenp

Never say "last" cleanup. The renaming to remove "lightweight" looks good. I wasn't sure if we wanted to keep that name or not. There's a lightweight NMT coming so this won't be confused with that anymore. I guess that's good.

coleenp · 2025-10-24T11:10:09Z

src/hotspot/share/runtime/synchronizer.cpp

+
+// -----------------------------------------------------------------------------
+// ConcurrentHashTable storing links from objects to ObjectMonitors
+class ObjectMonitorTable : AllStatic {


I guess after looking at this, it made sense to combine the lightweightSynchronizer code into synchronizer.cpp (should be ObjectSynchronizer.hpp/cpp). I wonder if the OM table code could be split out into its own file objectMontitorTable.hpp/cpp. I feel like synchronzer.hpp/cpp again does too many different things.

Since we are currently investigating the OM table elseware (see: JDK-8365493) I wote for not doing any OM table refactoring in this PR.

coleenp · 2025-10-24T11:14:49Z

src/hotspot/share/runtime/synchronizer.inline.hpp

    return read_monitor(mark);
  } else {
-    return LightweightSynchronizer::get_monitor_from_table(current, obj);
+    return ObjectSynchronizer::get_monitor_from_table(current, obj);


I don't think there's a need for this file anymore. read_monitor is mostly called inside synchronizer.cpp, so it can be inlined there.

Would you want me to do that in this PR? Or should I create a new RFE for that, just to acknowledge the fact that one should never say "last" cleanup. :)

bulasevich · 2025-10-24T13:23:44Z

run it on your favorite platform, and tell me if there's anything wrong with it. Thanks in advance.

Hi, tier1 runs clean on ARM32. Thanks!

fbredber · 2025-10-24T14:26:00Z

@TheRealMDoerr, @RealFYang

Indentation could be improved at many places where the next line was aligned (all platforms and shared code).

Oops, my bad. Guess this what you end up with when you use bash, grep -r and sed to seek out and remove "lightweight" from a source tree, and then check the result using old Un*x style diff -w. I think all the faulty indentation stuff is fixed now, and yes I did check with "new" style git diff to see the surrounding lines.

8367982: Unify ObjectSynchronizer and LightweightSynchronizer

ea3b583

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Oct 21, 2025

fbredber added 2 commits October 22, 2025 14:55

Small include line fix

e747ff9

Small arm32 fix

993d613

fbredber marked this pull request as ready for review October 23, 2025 08:32

openjdk bot added the rfr Pull request is ready for review label Oct 23, 2025

pchilano reviewed Oct 23, 2025

View reviewed changes

coleenp reviewed Oct 24, 2025

View reviewed changes

Update after review

6e33172

8367982: Unify ObjectSynchronizer and LightweightSynchronizer #27915

Are you sure you want to change the base?

8367982: Unify ObjectSynchronizer and LightweightSynchronizer #27915

Conversation

fbredber commented Oct 21, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Oct 21, 2025

Uh oh!

openjdk bot commented Oct 21, 2025

Uh oh!

openjdk bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

fbredber commented Oct 23, 2025

Uh oh!

TheRealMDoerr commented Oct 23, 2025

Uh oh!

pchilano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pchilano Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

fbredber Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

RealFYang commented Oct 24, 2025

Uh oh!

offamitkumar commented Oct 24, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbredber Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

fbredber Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulasevich commented Oct 24, 2025

Uh oh!

fbredber commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

fbredber commented Oct 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 21, 2025 •

edited

Loading

mlbridge bot commented Oct 23, 2025 •

edited

Loading

coleenp Oct 24, 2025 •

edited

Loading

fbredber Oct 24, 2025 •

edited

Loading

fbredber Oct 24, 2025 •

edited

Loading