JDK-8230459: Test failed to resume JVMCI CompilerThread

[David Holmes]

On 22/10/2019 11:18 pm, Doerr, Martin wrote:
> 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.

You didn't change line 946 as referenced above, you made changes to 
CompileBroker::init_compiler_sweeper_threads(). You don't need to make 
changes there as any issue is dealt with during VM init anyway. 
Additionally there are still many uses of CHECK in that function which 
would also need fixing. So I'd advise just leave that function alone.

But please fix CompileBroker::possibly_add_compiler_threads (line 956 in 
your current webrev).

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

I can't say I like the direct use of OopStorage and 
NativeAccess::oop_store. That seems to be breaking Handle encapsulation 
to me. I'd like to hear Kim Barrett's opinion on that in particular; or 
other runtime engineers.

David
-----

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