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

[David Holmes]

Hi Dan,

On 22/12/2018 7:35 am, Daniel D. Daugherty wrote:
> David,
> 
> Sorry for the delay in getting to this review.

No problem thanks for looking at it. I'm still waiting for Kim and he's 
already on Xmas vacation so no rush. I'll deal with this in January.

I'll take up your suggestions below.

Thanks,
David

> 
> On 12/19/18 4:21 PM, David Holmes 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/
> 
> src/hotspot/os/linux/os_linux.cpp
>      No comments.
> 
> src/hotspot/share/gc/parallel/gcTaskThread.cpp
>      Nit - needs a copyright year update.
> 
> src/hotspot/share/gc/shared/concurrentGCThread.cpp
>      Nit - needs a copyright year update.
> 
> src/hotspot/share/gc/shared/workgroup.cpp
>      No comments.
> 
> src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp
>      No comments.
> 
> src/hotspot/share/runtime/thread.cpp
>      L426:   assert(_run_state == PRE_CALL_RUN ||
>      L427:          _run_state == POST_RUN, "Active Thread deleted 
> before post_run()");
>          Adding a little more debug info would be helpful. Please consider:
> 
>            assert(_run_state == PRE_CALL_RUN ||
>                   _run_state == POST_RUN, "Active Thread deleted before 
> post_run(): "
>                   "_run_state=%d", (int)_run_state);
> 
>      L1082:   assert(_starting_thread == NULL, "already initialized");
>          Adding a little more debug info would be helpful. Please consider:
> 
>            assert(_starting_thread == NULL, "already initialized: "
>                   "_starting_thread=" INTPTR_FORMAT, _starting_thread);
> 
> src/hotspot/share/runtime/thread.hpp
>      L112-L135:
>          I like the content of the new comment block. I'm not fond of
>          using yet another block comment style:
> 
>            /*
>              <block of text>
>             */
> 
>          We already have:
> 
>            /*
>             * <block of text>
>             */
> 
>          and (my preference):
> 
>            //
>            // <block of text>
>            //
> 
> 
> src/hotspot/share/runtime/vmThread.cpp
>      No comments.
> 
> src/hotspot/share/services/management.cpp
>      No comments.
> 
> test/hotspot/gtest/threadHelper.inline.hpp
>      No comments.
> 
> Thumbs up. I don't need to see another webrev if you choose to
> fix the few things I noted above.
> 
> Dan
> 
> 
>>
>> Changes since previous discussion:
>>
>> - Replaced Kim's initial_thread_created assertion with a check of 
>> Thread::current_or_null, with an explanation
>>
>> - Removed NJT list addition at construction time as it's not needed 
>> after 8215097.
>>
>> - Moved the JavaThread call to this->exit(false); from the end of 
>> thread_main_inner to the start of post_run - as Markus suggested. This 
>> also necessitated a change to the gtest logic compared to the original 
>> as it has to override post_run() now to avoid the exit call.
>>
>> - Made get_thread_name_string virtual so that the JavaThread 
>> subclasses in the gtest can override it and allow their names to be 
>> seen in hs_err files. (Makes debugging so much easier when you know 
>> which thread crashed!).
>>
>> Testing:
>>  - Mach 5 tiers 1 - 3
>>  - All runtime tests with:
>>    - default GC
>>    - ZGC
>>    - Shenandoah GC
>> - gc/g1
>> - gc/serial
>> - gc/epsilon
>> - gc/shenandoah
>>
>> Thanks,
>> David
>