On Thu, 25 May 2023 05:02:19 GMT, David Holmes <dhol...@openjdk.org> wrote:
> We now track the in-progress and completed states of VM creation and only > return a VM from JNI_GetCreatedJavaVMs when there is a fully initialized VM. > > Testing: > - new regression test > - tiers 1-3 (sanity) > > Thanks Hi, over all looks good, but I have some questions regarding the tests and I wonder whether you missed a case. src/hotspot/share/prims/jni.cpp line 3475: > 3473: > 3474: // Global invocation API vars > 3475: enum VM_Creation_State { Nit: Would `enum class` be preferred here? src/hotspot/share/prims/jni.cpp line 3943: > 3941: // initialization, so only bail-out if something seems very wrong. > 3942: // Though how would we get here in that case? > 3943: if (vm_created == NOT_CREATED) { Shouldn't we also handle `IN_PROGRESS`? test/hotspot/jtreg/runtime/jni/getCreatedJavaVMs/exeGetCreatedJavaVMs.c line 1: > 1: /* Can we make this test behave even worse than it already does and have you caused a crash with this test before applying your changes? If it has caused a crash, then I think we're OK with the current form of the test. Two ideas to make it more likely to crash: 1. Have more threads racing 2. Let's say you spawn more threads, is it possible for `printf` to prevent data races as it's MT-Safe? More threads would increase likelihood of contention on the implicit lock, leading to some threads having been significantly slowed down by having to perform a more expensive locking procedure, is my thought process. ------------- PR Review: https://git.openjdk.org/jdk/pull/14139#pullrequestreview-1443519793 PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1205276576 PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1205284282 PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1205305164