RFR: 8268902: Testing for threadObj != NULL is unnecessary in handshake

[Coleen Phillimore]

On 6/22/21 6:59 PM, David Holmes wrote:
> On 23/06/2021 12:39 am, Coleen Phillimore wrote:
>> On Wed, 16 Jun 2021 16:03:04 GMT, Coleen Phillimore 
>> <coleenp at openjdk.org> wrote:
>>
>>> The handshake code tests if the JavaThread->is_exiting or that the 
>>> threadObj() is null. Ever since JDK-8244997, once the JavaThread is 
>>> running, the _threadObj won't be null until JavaThread is destroyed. 
>>> So testing is_exiting is all you need to do.
>>> In gtest, the test JavaThread doesn't create a _threadObj 
>>> JDK-8215948 so removing this unnecessary test allows writing gtests 
>>> for handshakes.
>>>
>>> Tested with tier1-6.
>>
>> I am looking at all the threads we create in the vm like the 
>> ServiceThread (look for Threads::add()), but not 
>> attach_current_thread in jni.  You're right, we have to make this 
>> thread safepoint here.
>> I wonder why some threads we create call ThreadGroup.add and some 
>> don't.  There's lots of duplicate code that's slightly different for 
>> each.
>
> I'm not sure on the "why". Some internal threads (e.g. AttachListener) 
> don't call the j.l.Thread constructor and so have to directly call the 
> ThreadGroup.add method. I suspect because these are not true Java 
> threads that there is some Java-side logic that Thread construction 
> performs which is not suitable for these internal threads.

Seems that ServiceTherad, MonitorDeflationThread and JFRRecorderThread 
do not call ThreadGroup.add, but NotificationThread, SignalThread (for 
-xrs), AttachListener, JNI attached threads, don't know JvmtiAgent 
threads, and JVM_StartThread (calls JavaThread::prepare).

There's lots of variations with similar code to create threads.
>
>> Unfortunately now I need a fix for JDK-8215948, hopefully without 
>> another duplicate copy of this thread creation code so I can write my 
>> handshake test.
>
> Okay guess I'd better look at it again then. :) I can determine which 
> variant of the j.l.Thread creation logic is suitable and then factor 
> that out into a support method like 
> Thread::create_internal_java_thread().

Please!  Refactoring would be very nice!

Thanks,
Coleen

>
> David
> -----
>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/4512
>>