JDK-8230459: Test failed to resume JVMCI CompilerThread

[Doerr, Martin]

Hi David and Kim,

I think it's easier to talk about code. So here's a new webrev:
http://cr.openjdk.java.net/~mdoerr/8230459_JVMCI_thread_objects/webrev.03/

@Kim:
Thanks for looking at the handle related parts. It's ok if you don't want to be a reviewer of the whole change.

> 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).
Nevertheless, it has the advantage that it enforces the update to be consistent.
A caller could use it without holding the lock or mess it up otherwise.
In addition, I don't what to change that as part of this fix.

> So far as I can tell, THREAD == NULL here.
This is a very tricky part (not my invention):
EXCEPTION_MARK contains an ExceptionMark constructor call which sets __the_thread__ to Thread::current().
I don't want to publish my opinion about this ��

@David:
Seems like this option is preferred over option 3 (possibly_add_compiler_threads part of webrev.02 and leave the initialization as is).
So when you're ok with it, I'll request a 2nd review from the compiler folks (I should change the subject to contain RFR).

Thanks,
Martin


> -----Original Message-----
> From: David Holmes <david.holmes at oracle.com>
> Sent: Montag, 28. Oktober 2019 05:04
> To: Kim Barrett <kim.barrett at oracle.com>
> Cc: Doerr, Martin <martin.doerr at sap.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 28/10/2019 1:42 pm, Kim Barrett wrote:
> >> On Oct 27, 2019, at 6:04 PM, David Holmes <david.holmes at oracle.com>
> wrote:
> >>
> >> 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/webr
> ev.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.
> >
> > I think if there is such a thread here that it can't die, because the
> > death predicate (the can_remove stuff) won't see that old thread as
> > the last thread in _compiler2_objects.  That's what I meant by this:
> >
> >> On Oct 25, 2019, at 4:05 PM, Kim Barrett <kim.barrett at oracle.com>
> wrote:
> >> 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.
> >
> > I think either that comment about an old thread is wrong (and the NULL
> > assertion I suggested is okay), or I think the whole mechanism here
> > has problems.  Or at least I was unable to figure out how it could work...
> >
> 
> I'm not following sorry. You can't assert NULL unless it's actually set
> to NULL which it presently isn't. But it could be set NULL as Martin
> suggested:
> 
> "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."
> 
> and which I also supported. But that aside once the delete_global has
> been called that JNIHandle no longer references the j.l.Thread that it
> did, at which point it is only reachable via the threadObj() of the
> CompilerThread.
> 
> David