On Wed, 18 Jun 2025 18:23:58 GMT, Coleen Phillimore <cole...@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.
>
> 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.

Removed in the latest commit.

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

Removed in the latest commit.

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

Removed in the latest commit.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161282585
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161282345
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161282101

Reply via email to