On Fri, 3 Apr 2026 21:27:56 GMT, Chris Plummer <[email protected]> wrote:
>> 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? > > One other thing to point out is that running with > DiagnoseSyncOnValueBasedClasses=2 was removed by > [JDK-8359437](https://bugs.openjdk.org/browse/JDK-8359437) (@toxaart), but > then for some reason Alex added it back in for > [JDK-8377845](https://bugs.openjdk.org/browse/JDK-8377845). I'm not sure why > he added it back in rather than just remove the comment that was missed by > [JDK-8359437](https://bugs.openjdk.org/browse/JDK-8359437). If the test is no longer using a `@ValueBased` class then there is no need for this regression test, as it no longer exercises the relevant runtime code paths. However this means that we also no longer test that EA with `DiagnoseSyncOnValueBasedClasses` on non-preview runs. >From what I can see, there is no plan to remove >`DiagnoseSyncOnValueBasedClasses` with JEP 401. So I wonder if we could not >just have this specific `ValueBased` test case be conditional enabled based on >enable-preview. Removing the `@ValueBased` testing test case regardless of `DiagnoseSyncOnValueBasedClasses` means we have less coverage of any regressions that might be introduced after JEP 401 in non-preview mode w.r.t. EA and `@ValueBased`. But if the consensus is that we want to remove the preview-mode illegal code from (non-preview mode) tests, then I see no reason to keep this test case either. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30560#discussion_r3043181629
