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

[David Holmes]

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.

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

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

> ———————————————————————————————————————
> 
> I also like Dan's suggestions.

Yep will implement them.

Thanks,
David

>