On Thu, 25 May 2023 05:02:19 GMT, David Holmes <[email protected]> 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
