On Fri, 26 May 2023 01:06:21 GMT, David Holmes <[email protected]> wrote:
>> 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`?
>
> I guess my comment was not clear enough. :) It is normal to get here while
> IN_PROGRESS due to calls from the JDK native code during initialization. So
> we only consider it an error to call then when NOT_CREATED.
Sloppy reading from me! That makes sense, and the code doing the work
(L3960-L3965) seems to take all precautions necessary to ensure that nothing
goes wrong if we're `IN_PROGRESS`.
Just a minor suggestion: "during VM initialization" could be "while VM
initialization is in progress" to mirror the language in the enum, but that's
up to you.
>> 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.
>
> The test crashes an unpatched VM. We only need the two threads. One will
> initiate the creation of the VM and the other then tries to attach to it. We
> want the second thread to call GetCreatedJavaVMs very quickly to make it more
> likely the VM is not initialized, but not so quickly that GetCreatedJavaVMs
> reports zero VMs (regardless of the patch). The time it takes the main thread
> to create the second thread seems to handle that nicely. Of course it depends
> on number of CPUs etc but on my local machine the test, and the original
> version from the JBS issue, reliably crashes every time before the fix.
Alright, then I'm very happy with this.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1206430443
PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1206432196
