On Sat, 2 Nov 2024 11:02:13 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Deoptimization with escape analysis can fail when trying to rematerialize >> objects as described in JDK-8227309. In this test this can happen in Xcomp >> mode in the framework of the test resulting in a test failure. Making the >> number of threads non-final avoids scalar replacement and thus the OOM >> during deopt. > > test/jdk/java/util/concurrent/locks/Lock/OOMEInAQS.java line 48: > >> 46: // Intentionaly non-final to avoid EA of the threads array in main >> which can cause this test to >> 47: // fail in Xcomp mode. >> 48: static int NTHREADS = 2; // intentionally not a scalable test; > 2 >> is very slow > > Hello Tom, I don't have the necessary knowledge of runtime compilers, so > consider this as drive-by questions than a review. > > On its own, the `static final` construct appears to be the correct one for > this field. Removing the `final` to address an escape analysis implementation > detail appears odd. > Do you know if the failure happens only in `-Xcomp` mode? Looking at > JDK-8342775 it wasn't clear to me that was the case. If it's happening only > in `-Xcomp` mode, perhaps due to additional work being done by the compiler > threads (?) and the fact that this test intentional runs with a very low > `-Xmx`, maybe we should just skip the test from `-Xcomp` mode instead of > changing the field declaration? We have several such tests which we skip in > `-Xcomp` mode by using: > > @requires (vm.compMode != "Xcomp") I think it's better to actually execute the test which is trying ensure that OOM is handled by library code in all configurations. Using final is just a stylistic choice in the harness so removing it to allow to test to run in more configurations seems like better than avoiding those configurations. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21745#discussion_r1896086136
