JDK-8230459: Test failed to resume JVMCI CompilerThread

[Nils Eliasson]

Hi Martin,

Not that this is your fault, but the compileBroker has crossed the line 
for the number of "#if INCLUDE_JVMCI" a long time ago. This file screams 
for a refactoring where JVMCI is hidden behind an abstraction layer.

I'm not going to request it out of this bug fix, but I can't leave it 
without comment.

Regards,

Nils


On 2019-10-23 16:47, 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.
>
> 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");
>
> 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.
>
>
> 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.)
>>