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

[David Holmes]

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

On 27/12/2018 2:41 pm, David Holmes wrote:
> Hi Kim,
> 
> On 27/12/2018 10:18 am, Kim Barrett wrote:
>>> On Dec 19, 2018, at 4:21 PM, David Holmes <david.holmes at oracle.com> 
>>> wrote:
>>>
>>> Following on from the preliminary RFR:
>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-December/035737.html 
>>>
>>>
>>> I've merged with Kim's changes from 8215097.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214097
>>> Webrev: http://cr.openjdk.java.net/~dholmes/8214097/webrev.v2/
>>
>> Looks good.
> 
> Thanks for looking at this again!
> 
>> A few minor things, for which I don't need a new webrev (unless you
>> decide to make the normalization of JavaTestThreads part of this
>> change; see comment for that class below).
>>
>> ------------------------------------------------------------------------------ 
>>
>> 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.

>> ------------------------------------------------------------------------------ 
>>
>> test/hotspot/gtest/threadHelper.inline.hpp
>>    51   VMThreadBlocker() { }
>>
>> Nit: addition of whitespace for the body is inconsistent with the
>> immediately following destructor definition, as well as elsewhere in
>> this file.
> 
> Thanks. Leftover from changes where I was trying to set the thread name 
> (and failing). Will fix.
> 
>> ------------------------------------------------------------------------------ 
>>
>> 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.

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

> 
>> ———————————————————————————————————————
>>
>> I also like Dan's suggestions.
> 
> Yep will implement them.
> 
> Thanks,
> David
> 
>>