On Thu, 25 May 2023 10:17:59 GMT, Johan Sjölen <[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. Thanks for looking at this @jdksjolen ! > 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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/14139#issuecomment-1563689278 PR Review Comment: https://git.openjdk.org/jdk/pull/14139#discussion_r1206133926
