8359437: Make users and test suite not able to set LockingMode flag #25847

toxaart · 2025-06-17T08:39:49Z

This PR contains changes for the 1st phase of the LockingMode flag obsoletion.

The work is done by @fbredber, I have taken it over and am finishing it while he's on vacation.

In the 1st phase one keeps the LockingMode variable in all places, but makes it non-settable from the command line. All the C1 and C2 code related to legacy locking will still be in place (but as dead code) and removed later (phase 2).

Lightweight locking is the default locking from now on.

Tested in tiers 1 - 7.

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-8359437: Make users and test suite not able to set LockingMode flag (Sub-task - P4) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.

Contributors

Fredrik Bredberg <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25847

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

Using diff file

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

…e_obsoletion

toxaart · 2025-06-17T08:45:50Z

/help

bridgekeeper · 2025-06-17T08:49:56Z

👋 Welcome back toxaart! 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-06-17T08:50:23Z

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

openjdk · 2025-06-17T08:50:54Z

@toxaart Available commands:

approval - request for maintainer's approval
approve - null
author - sets an overriding author to be used in the commit when the PR is integrated
backport - create a backport
cc - add or remove an additional classification label
clean - Mark the backport pull request as a clean backport
contributor - adds or removes additional contributors for a PR
covered - used when employer has signed the OCA
csr - require a compatibility and specification request (CSR) for this pull request
help - shows this text
integrate - performs integration of the changes in the PR
issue - edit the list of issues that this PR solves
jep - require a JDK Enhancement Proposal (JEP) for this pull request
keepalive - Re-evaluates the pull request and resets the inactivity timeout.
label - add or remove an additional classification label
open - Set the pull request state to "open"
reviewer - manage additional reviewers for a PR
reviewers - set the number of additional required reviewers for this PR
signed - used after signing the OCA
solves - edit the list of issues that this PR solves
sponsor - performs integration of a PR that is authored by a non-committer
summary - updates the summary in the commit message
test - used to run tests
touch - Re-evaluates the pull request and resets the inactivity timeout.

openjdk · 2025-06-17T08:51:27Z

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

core-libs
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.

toxaart · 2025-06-17T08:58:50Z

/contributor add @fbredber

openjdk · 2025-06-17T08:59:57Z

@toxaart
Contributor Fredrik Bredberg <[email protected]> successfully added.

dholmes-ora

I've taken an initial pass through. Initially I misunderstood the strategy with heavy monitors - see comments below.

dholmes-ora · 2025-06-18T07:37:31Z

src/hotspot/share/runtime/arguments.cpp

  if (LockingMode == LM_LEGACY) {
-    FLAG_SET_CMDLINE(LockingMode, LM_LIGHTWEIGHT);
+    LockingMode = LM_LIGHTWEIGHT;


If we have prevented the locking mode from being set then surely we can never encounter this case?

dholmes-ora · 2025-06-18T07:41:46Z

test/hotspot/jtreg/runtime/Monitor/StressWrapper_TestRecursiveLocking_36M.java

@@ -33,21 +33,6 @@
 * @run main/othervm/timeout=240 -Xbootclasspath/a:.
 *     -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
 *     -Xint
- *     -XX:LockingMode=0


I was wondering why these LockingMode=0 test cases were not setting VerifyHeavyMonitors instead, but I'm assuming the intent now is that we will only test that mode when it is set externally by the user (or in our case a particular test task definition)?

I also realized we can only test heavy monitors in tests where we explicitly control the monitor creation places and hence can call the WB method to force inflation. That obviously reduces the test coverage for that mode quite significantly - but perhaps that will be handled if in the future we implicitly reenable forced inflation and do away with the WB usage.

dholmes-ora · 2025-06-18T07:52:02Z

test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java

This seems to remove significant test coverage. can we not adapt the tests to not rely on logging warnings that will no longer be present?

The premise of this test is now invalid. We could write a fresh new test if we'd like to see what happens with UseHeavyMonitors, and/or retrieve this from git history.

dholmes-ora · 2025-06-18T07:58:37Z

src/hotspot/share/utilities/globalDefinitions.cpp

@@ -56,6 +56,8 @@ int LogMinObjAlignmentInBytes  = -1;
 // Oop encoding heap max
 uint64_t OopEncodingHeapMax = 0;

+int LockingMode = LM_LIGHTWEIGHT;


dholmes-ora · 2025-06-18T07:58:48Z

src/hotspot/share/utilities/globalDefinitions.hpp

@@ -1009,6 +1009,8 @@ enum LockingMode {
  LM_LIGHTWEIGHT = 2
 };

+extern int LockingMode;


coleenp

I have a request for some more deieting.

coleenp · 2025-06-18T18:23:58Z

test/hotspot/jtreg/runtime/Monitor/TestRecursiveLocking.java

@@ -212,11 +122,8 @@

 public class TestRecursiveLocking {
    static final WhiteBox WB = WhiteBox.getWhiteBox();
-    static final int flagLockingMode = WB.getIntVMFlag("LockingMode").intValue();
+    static final boolean flagHeavyMonitors = WB.getBooleanVMFlag("VerifyHeavyMonitors");


I think you should take out the VerifyHeavyMonitors cases. @fbredber originally had that flag turn on a the reintroduced UseHeavyMonitors option but the UseHeavyMonitors option doesn't actually do that with this change. I don't think this test will pass with -XX:+VerifyHeavyMonitors.
If we reintroduce UseHeavyMonitors, save this diff and fix this test then. Right now it's not correct.

coleenp · 2025-06-18T18:31:55Z

test/hotspot/jtreg/runtime/lockStack/TestLockStackCapacity.java

@@ -39,8 +39,7 @@

 public class TestLockStackCapacity {
    static final WhiteBox WB = WhiteBox.getWhiteBox();
-    static final int LockingMode = WB.getIntVMFlag("LockingMode").intValue();
-    static final int LM_LIGHTWEIGHT = 2;
+    static final boolean flagHeavyMonitors = WB.getBooleanVMFlag("VerifyHeavyMonitors");


I think this should also not have cases for VerifyHeavyMonitors. We can add back tests if we want UseHeavyMonitors. As of now, removing the Legacy locking code will remove code that reaches the VerifyHeavyMonitors branches.

coleenp · 2025-06-18T18:34:12Z

test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java

The premise of this test is now invalid. We could write a fresh new test if we'd like to see what happens with UseHeavyMonitors, and/or retrieve this from git history.

coleenp · 2025-06-18T18:38:21Z

test/jtreg-ext/requires/VMProps.java

@@ -423,10 +423,11 @@ protected String vmHasDTrace() {
     */
    protected String vmRTMCompiler() {


There's an issue to remove this function since it's now unused.

fbredber and others added 4 commits June 5, 2025 13:15

First try.

d0d547e

Update after pre-review

775c30a

Merge branch 'master' into _remove_locking_mode_fix_tests

bf2a8bf

Merge remote-tracking branch 'origin/master' into 8359437_locking_mod…

962a709

…e_obsoletion

openjdk bot added serviceability [email protected] hotspot [email protected] core-libs [email protected] labels Jun 17, 2025

dholmes-ora reviewed Jun 18, 2025

View reviewed changes

coleenp suggested changes Jun 18, 2025

View reviewed changes

		@@ -423,10 +423,11 @@ protected String vmHasDTrace() {
		*/
		protected String vmRTMCompiler() {

8359437: Make users and test suite not able to set LockingMode flag #25847

Are you sure you want to change the base?

8359437: Make users and test suite not able to set LockingMode flag #25847

Conversation

toxaart commented Jun 17, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Contributors

Reviewing

Uh oh!

toxaart commented Jun 17, 2025

Uh oh!

bridgekeeper bot commented Jun 17, 2025

Uh oh!

openjdk bot commented Jun 17, 2025

Uh oh!

openjdk bot commented Jun 17, 2025

Uh oh!

openjdk bot commented Jun 17, 2025

Uh oh!

toxaart commented Jun 17, 2025

Uh oh!

openjdk bot commented Jun 17, 2025

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toxaart commented Jun 17, 2025 •

edited by openjdk bot

Loading