On Fri, 3 Apr 2026 02:13:03 GMT, Leonid Mesnik <[email protected]> wrote:
>> com/sun/jdi/EATests.java synchronizes on an Integer instance. Although this >> currently works, it is discouraged. See the following doc on value based >> classes: >> >> https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/doc-files/ValueBased.html >> >> "Synchronization on instances of value-based classes is strongly >> discouraged, because the programmer cannot guarantee exclusive ownership of >> the associated monitor. >> >> Identity-related behavior of value-based classes may change in a future >> release. For example, synchronization may fail." >> >> That second part is realized by Valhalla. The synchronization fails with: >> >> `java.lang.IdentityException: Cannot synchronize on an instance of value >> class java.lang.Integer` >> >> Valhalla CR [JDK-8372831](https://bugs.openjdk.org/browse/JDK-8372831) >> covers that failure, but I thought it best to address this in mainline first. >> >> Tested with all of CI tier1, tier2 svc, and tier5 svc. > > test/jdk/com/sun/jdi/EATests.java line 2411: > >> 2409: * Test relocking eliminated @ValueBased object. >> 2410: */ >> 2411: class EARelockingObject extends EATestCaseBaseDebugger { > > The goal of testcase was to specifically lock on the ValueBased object. > I think this testacase now doing the same as EARelockingSimple and the best > fix would be just to delete this testcase. You are right. I thought there might be some reason to keep it in, but I see now that it basically duplicates EARelockingSimple. It looks like we are losing some useful testing by not syncing on Integer. In #7626, which added the test, the description says: "Can reproduce a crash by modifying test/jdk/com/sun/jdi/EATests.java and using -XX:DiagnoseSyncOnValueBasedClasses=2 with LM_LEGACY or running test/jdk/com/sun/jdi/EATests.java with LM_LIGHTWEIGHT/LM_MONITOR and -Xlog:monitorinflation=trace." So we would be losing this testing, which seems like it might be important. Eventually it will have to go away when Valhalla moves out of preview. We could keep it for now and only run it when PreviewFeatures.isEnabled() returns false, but eventually Valhalla will be out of preview and this subtest will need to be removed. @xmas92 Do you think it is useful to keep EARelockingValueBased around until Valhalla is out of preview, or should we just get rid of it now? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30560#discussion_r3031221853
