JDK-8230459: Test failed to resume JVMCI CompilerThread

Tue Oct 22 13:18:47 UTC 2019

Hi David,

> Okay. The preexisting:
> 
> 946       JavaThread *ct = make_thread(compiler2_object(i),
> _c2_compile_queue, _compilers[1], CHECK);
> 
> 
> should have a bug filed to have it fixed as well.

Since I'm touching this code with this change, I just fixed it. I guess this problem is rather a theoretical one.

> 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/webrev.02/

Best regards,
Martin

> -----Original Message-----
> From: David Holmes <david.holmes at oracle.com>
> Sent: Dienstag, 22. Oktober 2019 08:15
> To: Doerr, Martin <martin.doerr at sap.com>
> Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
> dev at openjdk.java.net>; Vladimir Kozlov (vladimir.kozlov at oracle.com)
> <vladimir.kozlov at oracle.com>; dean.long at oracle.com; David Holmes
> <David.Holmes at oracle.com>
> Subject: Re: JDK-8230459: Test failed to resume JVMCI CompilerThread
> 
> Hi Martin,
> 
> On 22/10/2019 4:02 am, Doerr, Martin wrote:
> > Hi David,
> >
> >> I have no idea why you made any change to
> >> CompileBroker::init_compiler_sweeper_threads() as there are no issues
> at
> >> initialization time as that is done by the main thread during VM init. ??
> >
> > This part of the change is not strictly necessary, but otherwise, we'd create
> thread objects which would never be used.
> > We only need one JVMCI thread object during init.
> possibly_add_compiler_threads() creates new ones directly before using
> them (on demand).
> 
> Okay.
> 
> >
> >> For the changes in CompileBroker::possibly_add_compiler_threads() I
> >> would suggest adding an initial comment to replace what you have here:
> >>
> >>    921         // C1 threads can't create thread objects.
> >>    923         // JVMCI compilers are allowed to call Java for creating
> >> thread objects.
> >>    924         // They should not reuse old ones because unexpected
> >> thread state transitions would be observable.
> >>
> >> to more clearly explain things i.e.:
> >>
> >> for (int i = old_c2_count; i < new_c2_count; i++) {
> >>     // Native compiler threads as used in C1/C2 can reuse the j.l.Thread
> >>     // objects as their existence is completely hidden from the rest of
> >>     // the VM (and those compiler threads can't call Java code to do the
> >>     // creation anyway). For JVMCI we have to create new j.l.Thread
> objects
> >>     // as they are visible and we can see unexpected thread lifecycle
> >> transitions
> >>     // if we bind them to new JavaThreads.
> >>    #if INCLUDE_JVMCI
> >>    ...
> >>
> >> ---
> >
> > Sure, I can replace the comment.
> 
> Thanks.
> 
> >
> >>    922         if (!CompilerThread::current()->can_call_java()) break;
> >>
> >> Just want to confirm this code can only ever be called by compiler
> threads?
> >
> > It's only called by compiler_thread_loop which asserts this.
> 
> Okay.
> 
> >
> >> Also unclear if UseJVMCICompiler is true, how we might have a compiler
> >> thread that can't call Java ??
> >
> > With TieredCompilation, C1 compiler threads also adds C2 threads if their
> queue is long.
> > However, only JVMCI threads can create new JVMCI threads if they have
> to create thread objects.
> 
> Okay - It wasn't clear to me that JVMCI threads and native compiler
> threads can co-exist in the same incarnation of the VM.
> 
> >
> >> 926         sprintf(name_buffer, "%s CompilerThread%d",
> >> _compilers[1]->name(), i);
> >>
> >> I'm wondering if we should make the name more unique rather than
> reusing
> >> the same name as the old Thread had? This would make debugging
> >> somewhat clearer I think.
> >
> > Not sure if gaps between compiler thread numbers would be expected /
> desired.
> > But if all reviewers prefer that, I can change it.
> 
> The flip side of that is if you see CompilerThread2 in logging output
> etc and try to reason about things you could get very confused if it was
> actually entirely different threads. But I'll defer to compiler folk as
> they will be the ones that have to deal with that confusion.
> 
> >
> >>    927         Handle thread_oop = create_thread_oop(name_buffer,
> CHECK);
> >>
> >> This use of CHECK, and the preexisting use of it with make_thread, is
> >> wrong! There is an EXCEPTION_MARK at the start of this method. If either
> >> of these calls result in an exception then it will cause a VM abort. I
> >> would expect in this case that if any exception occurs we should just
> >> clear it, possibly logging it first, and then abandon the operation.
> >> Further if this code is being executed by native compiler threads then
> >> exceptions are impossible as they couldn't have executed the Java code
> >> that would post them! So something seems amiss with regards to
> >> exceptions and this code (and what it calls).
> >
> > Right. We should clear the exception and abort the JVMCI thread creation.
> 
> Okay. The preexisting:
> 
> 946       JavaThread *ct = make_thread(compiler2_object(i),
> _c2_compile_queue, _compilers[1], CHECK);
> 
> 
> should have a bug filed to have it fixed as well.
> 
> >
> >>    929         _compiler2_objects[i] = thread_handle; // Old one dies
> >> here if it exists.
> >>
> >> You need to call  JNIHandles::destroy_global on the old handle if it exists.
> >
> > I don't want to release the handle. Just to replace the thread object in the
> handle.
> > Comment updated.
> 
> 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.
> 
> Thanks,
> David
> -----
> 
> >
> > New webrev:
> >
> http://cr.openjdk.java.net/~mdoerr/8230459_JVMCI_thread_objects/webr
> ev.01/
> >
> >
> > Best regards,
> > Martin
> >