JDK-8230459: Test failed to resume JVMCI CompilerThread

[David Holmes]

Hi Martin,

On 16/10/2019 11:09 pm, Doerr, Martin wrote:
> Hi David,
> 
> thanks for confirming.
> 
> So I suggest to fix it like this:
> http://cr.openjdk.java.net/~mdoerr/8230459_JVMCI_thread_objects/webrev.00/

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

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

---

  922         if (!CompilerThread::current()->can_call_java()) break;

Just want to confirm this code can only ever be called by compiler threads?

Also unclear if UseJVMCICompiler is true, how we might have a compiler 
thread that can't call Java ??

---

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.

---

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

---

  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.

Thanks,
David
-----

> Best regards,
> Martin
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Mittwoch, 16. Oktober 2019 09:11
>> To: Doerr, Martin <martin.doerr at sap.com>
>> Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
>> dev at openjdk.java.net>; David Holmes <David.Holmes at oracle.com>
>> Subject: Re: JDK-8230459: Test failed to resume JVMCI CompilerThread
>>
>> Hi Martin,
>>
>> On 15/10/2019 12:50 am, Doerr, Martin wrote:
>>> Hi David,
>>>
>>>>> As C1 and C2 compiler threads are hidden, I still don't see any problems
>> reusing the Thread objects.
>>>
>>>> They are not completely hidden. Those j.l.Thread objects are still part of
>> the system thread group and can be found by other code.
>>>
>>> I know that they are part of the system thread group, but I thought they
>>> should not be visible to Java.
>>>
>>> E.g.
>>>
>>>           ThreadGroup tg = new ThreadGroup("A");
>>>           ThreadGroup stg = tg.getParent().getParent();
>>>           Thread[] list = new Thread[100];
>>>           stg.enumerate(list);
>>>           System.out.println("Listing elements of " + stg);
>>>           for (int i = 0; i < list.length; ++i) {
>>>               if (list[i] != null) {
>>>                   System.out.println(i + ": " + list[i]);
>>>               }
>>>           }
>>>
>>> Finds:
>>>
>>> Listing elements of java.lang.ThreadGroup[name=system,maxpri=10]
>>>
>>> 0: Thread[Reference Handler,10,system]
>>> 1: Thread[Finalizer,8,system]
>>> 2: Thread[Signal Dispatcher,9,system]
>>> 3: Thread[main,5,main]
>>>
>>> I can't see any compiler threads.
>>
>> Yes you are right. I had forgotten about this other quirk in this whole
>> arrangement. Threads started directly by the VM, rather than via
>> Thread.start, don't call ThreadGroup.add unless the VM explicitly makes
>> that call - which it does for the Signal Dispatcher thread and a couple
>> of others. The compiler threads aren't included so while they consider
>> themselves to be part of the system ThreadGroup, from the group's
>> perspective they are "unstarted threads" - something which is itself
>> somewhat of an oddity. But that does mean they are not discoverable that
>> way.
>>
>>> Do you mean finding them by JVMTI agent?
>>>
>>> As I understand the Spec, compiler threads should be excluded for
>>> GetAllThreads: "The threads are Java programming language threads"
>>
>> They are hidden from JVMTI (unlike the JVMCI compiler threads)
>>
>>> Or what did you mean by "other code" which can see C1 and C2 threads?
>>
>> I did mean by enumerating the system ThreadGroup.
>>
>> So for C1/C2 compiler threads we can probably get away with reusing the
>> j.l.Thread object.
>>
>> Thanks,
>> David
>> -----
>>
>>> Best regards,
>>>
>>> Martin
>>>