JDK-8230459: Test failed to resume JVMCI CompilerThread

[Doerr, Martin]

Hi Kim,

> src/hotspot/share/compiler/compileBroker.cpp
>  948         assert(compiler2_object(i) = NULL, "Old one must be released!”);
> 
> That should be “==“ rather than “=“.
Nice one �� Fixed.

Alright, thanks again for your input related to the handle usage. I guess we don't need to keep you in the loop for the other stuff.

Best regards,
Martin


> -----Original Message-----
> From: Kim Barrett <kim.barrett at oracle.com>
> Sent: Montag, 28. Oktober 2019 20:40
> 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 28, 2019, at 10:06 AM, Doerr, Martin <martin.doerr at sap.com>
> wrote:
> > @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’ll reply to the pending parts, and drop out.  I think the handle related parts
> are
> going in the right direction now.
> 
> >> 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.
> 
> Understood, though I think I recall the caller with do_it = true is holding the
> appropriate lock
> around the call.
> 
> >> 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 ��
> 
> Oh, that’s interesting…  I know exceptions.hpp contains some exciting code,
> but I’ve so
> far avoided looking at it closely.  That stuff doesn’t come up much in gc code
> :)
> 
> > @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).
> 
> Handle usage here looks okay to me now.  I think JVMCI global handles could
> be used instead,
> but I’ll leave that up to others.