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

[Daniel D. Daugherty]

David,

Sorry for the delay in getting to this review.


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