RFR: JDK-8214097: Rework thread initialization and teardown logic
David Holmes
david.holmes at oracle.com
Fri Dec 28 01:20:01 UTC 2018
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
>
>>
More information about the hotspot-dev
mailing list