RFR: JDK-8214097: Rework thread initialization and teardown logic

[David Holmes]

On 28/12/2018 3:25 pm, Kim Barrett wrote:
>> On Dec 27, 2018, at 8:20 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> A couple of follow ups below.
>>
>> Changes applied at:
>>
>> http://cr.openjdk.java.net/~dholmes/8214097/webrev.v3/
>>
>> I'll be pushing this once basic testing is complete. (Don't want to have to update all the copyright notices to 2019 :) )
> 
> Good plan!
> 
>> On 27/12/2018 2:41 pm, David Holmes wrote:
>>> Hi Kim,
>>> On 27/12/2018 10:18 am, Kim Barrett wrote:
>>>> src/hotspot/share/runtime/thread.hpp
>>>>    857 // [...] There is a special case during VM startup
>>>>    858 // when the BarrierSet has not yet been created, where we add to the
>>>>    859 // list during the constructor. [...]
>>>>
>>>> That "special case" no longer exists.
>>> There's still the special case of the initial thread. I'll double check the context and update as needed.
>>
>> I was confusing my "special cases" - this is nothing to do with main thread. So I rewrote the comment to be much smaller and simpler.
> 
> Yes, that’s better.  Thanks.
> 
>>>> test/hotspot/gtest/threadHelper.inline.hpp
>>>>     98     return (char*) "JavaTestThread";
>>>>
>>>> Casting away const is unnecessary since the return type is "const
>>>> char*".  (And casting away const of a string literal isn't such a
>>>> great idea anyway.)
>>> Okay. This was a direct copy of Thread::name, so for consistency I'll fix that too.
>>
>> Actually the return type is only "char*" not "const char*" (but it is a const function), so I couldn't make this change. Trying to make the function return const char* also had a fan out affect, so I just left as-is.
> 
> ??? The signature you just checked in is:
> 
> 97: const char* get_thread_name_string(char* buf, int buflen) const {

Sorry managed to totally confuse myself. I was in a time warp thinking 
I'd overridden Thread::name(), but that was the approach that doesn't 
work because for JavaThreads we call get_thread_name() and thus 
get_thread_name_string(). So I mistakenly tried to remove the cast in 
Thread::name first, which fails because it doesn't return const char*, 
and so left it as-is. <sigh>

I'll do a follow up.

David
-----

>>>> test/hotspot/gtest/threadHelper.inline.hpp
>>>>
>>>> These JavaTestThreads are weird, and it seems like it would be better
>>>> if they were more normal.  But maybe that can be done in a later
>>>> cleanup.  The new, improved, life-cycle management might make that
>>>> easier.
>>> I don't think they really can be "normal" JavaThreads as that requires they get a java.lang.Thread object and all the associated logic that goes along with that. I know little of the gtest mechanism and have no idea how much of the VM is initialized when these gtests run. Anyway this is out of scope for this change.
>>
>> This may actually be simpler than I thought if we just do as we do for the "service thread". I filed:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8215948
>>
>> and assigned to Robbin for consideration.
> 
> Thanks.
>