On Wed, 18 Jun 2025 07:37:31 GMT, David Holmes <dhol...@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.
>
> src/hotspot/share/runtime/arguments.cpp line 1839:
>
>> 1837: #ifndef _LP64
>> 1838: if (LockingMode == LM_LEGACY) {
>> 1839: LockingMode = LM_LIGHTWEIGHT;
>
> If we have prevented the locking mode from being set then surely we can never
> encounter this case?
Looks like yes, this whole check then can be removed. Addressed in the latest
commit.
> src/hotspot/share/utilities/globalDefinitions.cpp line 59:
>
>> 57: uint64_t OopEncodingHeapMax = 0;
>> 58:
>> 59: int LockingMode = LM_LIGHTWEIGHT;
>
> const ?
This can be done provided that one removes assignment on line 3763 in
arguments.cpp. That assignment looks redundant as LockingMode is always
LM_LIGHTWEIGHT from now on.
> src/hotspot/share/utilities/globalDefinitions.hpp line 1012:
>
>> 1010: };
>> 1011:
>> 1012: extern int LockingMode;
>
> const ?
This can be done provided that one removes assignment on line 3763 in
arguments.cpp. That assignment looks redundant as LockingMode is always
LM_LIGHTWEIGHT from now on.
> test/hotspot/jtreg/runtime/Monitor/StressWrapper_TestRecursiveLocking_36M.java
> line 36:
>
>> 34: * -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
>> 35: * -Xint
>> 36: * -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.
My understanding is that VerifyHeavyMonitors requires LockingMode = 0, see line
1852 of arguments.cpp. So one has to set both at the same time, not one instead
of another. Now locking mode is hardcoded to lightweight, and there is no way
to use the incompatible `VerifyHeavyMonitors` option.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161284370
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161120254
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161119703
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161284143
