JDK-8230459: Test failed to resume JVMCI CompilerThread

[Doerr, Martin]

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.)
>