JDK-8230459: Test failed to resume JVMCI CompilerThread

[David Holmes]

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/webrev.01/
> 
> 
> Best regards,
> Martin
>