On Tue, 17 Jun 2025 08:39:49 GMT, Anton Artemov <d...@openjdk.org> wrote:

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

I have a request for some more deieting.

This looks great. Thank you!

test/hotspot/jtreg/runtime/Monitor/TestRecursiveLocking.java line 125:

> 123: public class TestRecursiveLocking {
> 124:     static final WhiteBox WB = WhiteBox.getWhiteBox();
> 125:     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.

test/hotspot/jtreg/runtime/lockStack/TestLockStackCapacity.java line 42:

> 40: public class TestLockStackCapacity {
> 41:     static final WhiteBox WB = WhiteBox.getWhiteBox();
> 42:     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.

test/jtreg-ext/requires/VMProps.java line 424:

> 422:      * Note: Lightweight locking does not support RTM (for now).
> 423:      */
> 424:     protected String vmRTMCompiler() {

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

-------------

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2940262141
Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2949835857
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2155234150
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2155246651
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2155256340

Reply via email to