JDK-8230459: Test failed to resume JVMCI CompilerThread

[David Holmes]

On 24/10/2019 12:47 am, Doerr, Martin wrote:
> Hi Kim,
> 
> I didn't like using the OopStorage stuff directly, either. I just have not seen how to allocate a global handle and add the oop later.
> 
> Thanks for pointing me to JVMCI::make_global. I was not aware of that.
> 
> So I can imagine 3 ways to implement it:
> 
> 1. Use JNIHandles::destroy_global to release the handles. I just added that to
> http://cr.openjdk.java.net/~mdoerr/8230459_JVMCI_thread_objects/webrev.01/
> We may want to improve that further by setting the handle pointer to NULL and asserting that it is NULL before adding the new one.
> I had been concerned about NULLs in the array, but looks like the existing code can deal with that.

I think it would be cleaner to both destroy the global handle and NULL 
it in the array at the same time.

This comment

325         // Old j.l.Thread object can die here.

Isn't quite accurate. The j.l.Thread is still referenced via 
ct->threadObj() so can't "die" until that is also cleared during the 
actual termination process.

The use of the CHECK macro at existing places in 
possibly_add_compilerthreads still needs to be fixed. The correct 
exception handling in your new code just highlights the deficiency of 
the existing code.

> 2. Use JVMCI::make_global, keep the handles and replace the j.t.Thread objects. Changed in place:
> http://cr.openjdk.java.net/~mdoerr/8230459_JVMCI_thread_objects/webrev.02/
> 
> JMVCI::make_global uses OopStorageSet::vm_global instead of jni_global. No clue if this is better or worse.
> 
> But seems like JMVCI::make_global still doesn't like NULL handles in slowdebug build ��
> assert(oopDesc::is_oop(obj()), "not an oop");

I don't see how this code is an improvement on anything. It duplicates 
some existing code as JVMCI threads now have to "make global" in a 
different way; and it still directly uses NativeAccess<>::oop_store 
which breaks encapsulation.

> 3. Just use the possibly_add_compiler_threads part of webrev.02 and leave the initialization as is.
> This means we create j.l.Thread objects initially and replace them by new ones when the JVMCI threads get started. I'd be ok with this, too.

I much prefer simple direct use of JNIHandles::destroy_global as per 
version 1 above.

Thanks,
David
------

> 
> Sorry for the in-place updates, but so many webrevs would confuse me after some time.
> 
> 
> Best regards,
> Martin
> 
> 
>> -----Original Message-----
>> From: Kim Barrett <kim.barrett at oracle.com>
>> Sent: Mittwoch, 23. Oktober 2019 02:25
>> To: Doerr, Martin <martin.doerr at sap.com>
>> Cc: David Holmes <david.holmes at oracle.com>; Vladimir Kozlov
>> (vladimir.kozlov at oracle.com) <vladimir.kozlov at oracle.com>; hotspot-
>> compiler-dev at openjdk.java.net
>> Subject: Re: JDK-8230459: Test failed to resume JVMCI CompilerThread
>>
>>> On Oct 22, 2019, at 9:18 AM, Doerr, Martin <martin.doerr at sap.com>
>> wrote:
>>>> Sorry I don't follow. The initial Handle stored in the array was a
>>>> global handle. You just allocated a second global Handle. I don't see
>>>> any operator=() magic happening here to free one of those global
>>>> handles, so AFAICS you need to free one of them.
>>>
>>> Sorry. I had implemented it differently than I wanted. Thanks for pointing
>> me to it.
>>>
>>> Fixed:
>>>
>> http://cr.openjdk.java.net/~mdoerr/8230459_JVMCI_thread_objects/webr
>> ev.02/
>>
>> Please don't do that. That's basically smashing through the JNHHandles
>> abstraction and writing a bunch of (lower level and tightly coupled)
>> code for no good reason. Just use JNIHandles::make_global and
>> JNHHandles::destroy_global as David suggested.
>>
>> (Note that JNIHandles::make_global takes an AllocFailType argument; if
>> you aren't going to handle allocation failure (e.g. are just going to
>> call vm_exit_out_of_memory, as in webrev.02) the default will deal
>> with that for you.)
>>
>> Alternatively, can you use JVMCI::make_global and JVMCI::destroy_global
>> here?  Perhaps not, if some of this code is !UseJVMCICompiler or whatever.
>> That's kind of unfortunate.  (It's also unfortunate that those functions
>> deal with jobject, but that's a different problem.)
>>
>