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've taken an initial pass through. Initially I misunderstood the strategy with
heavy monitors - see comments below.
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?
src/hotspot/share/utilities/globalDefinitions.cpp line 59:
> 57: uint64_t OopEncodingHeapMax = 0;
> 58:
> 59: int LockingMode = LM_LIGHTWEIGHT;
const ?
src/hotspot/share/utilities/globalDefinitions.hpp line 1012:
> 1010: };
> 1011:
> 1012: extern int LockingMode;
const ?
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.
test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java line 1:
> 1: /*
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?
-------------
PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2938076106
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153873165
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153924585
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153924946
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153884907
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153911036
