JDK-8230459: Test failed to resume JVMCI CompilerThread

[Kim Barrett]

> On Oct 23, 2019, at 10:47 AM, Doerr, Martin <martin.doerr at sap.com> wrote:
> 
> Hi Kim,

Note This isn't code that I'm familiar with, nor do I really want to
spend the time to become so right now, e.g. don't count me as any kind
of reviewer in this thread right now.  I'm just here because of the
questions about JNIHandles / OopStorage usage.

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

That's not a supported usage model.  It hasn't been needed, and might
not even be possible to support within the concurrency requirements
for OopStorage.  (I don't actually know whether it's possible now.
I'm pretty sure it wasn't in some of the prototypes.)  And I don't
think it is needed or helpful with the problem at hand either.

> 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's weird that can_remove() is a predicate with optional side
effects.  I think it would be simpler to have it be a pure predicate,
and have the one caller with do_it = true perform the updates.  That
should include NULLing out the handle pointer (perhaps debug-only, but
it doesn't cost much to cleanly maintain the data structure).

But regardless of that possible refactoring, I don't see how this can
work.

 934         if (!THREAD->can_call_java()) break;

So far as I can tell, THREAD == NULL here.

I also think that here:

 947         jobject thread_handle = JNIHandles::make_global(thread_oop);
 948         _compiler2_objects[i] = thread_handle;

should assert _compiler2_objects[i] == NULL.  Or if that isn't a valid
assertion then I think there are other problems.

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

The separation between the JNIHandles oopstorages and the vm_global /
vm_weak oopstorages exists to make it a bit less trivial for native
code to use JNI facilities to corrupt VM-internal data structures.
It's unfortunate for that purpose that JVMCI uses jobject though.

> But seems like JMVCI::make_global still doesn't like NULL handles in slowdebug build ��
> assert(oopDesc::is_oop(obj()), "not an oop”);

JVMCI::make/destroy_global expect to be used with actual oops, and
don't provide the NULL oop => NULL jobject mechanism provided by
JNIHandles.

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