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

[David Holmes]

Hi Markus,

On 21/12/2018 1:07 am, Markus Gronlund wrote:
> Thanks David for doing this work.
> 
> Looks good.

Thanks for looking at this.

> "
> - 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."
> 
> Just so that I understand the reason for the updates to the gtest part:
> 
> The reason you need to add a post_run() specialization in threadHelper.inline.hpp, is because the JavaThread::post_run() now invokes JavaThread::exit() (thanks for accommodating that btw), and JavaThread::exit() expects the thread to have a valid thread oop, something that the gtests do not setup/create?
> 
> JavaThread::exit() contains assertions like:
> ...
>    assert(threadObj.not_null(), "Java thread object should be created");

That's correct. And without a Thread oop, and being a real Java thread, 
little else in JavaThread::exit makes any sense.

> Nit:
> 
> Maybe the comment in threadHelper.inline.hpp:
> 
> // override as JavaThread::post_run() calls Thread::exit
> 
> Could become
> 
> // override as JavaThread::post_run() calls JavaThread::exit() which expect a valid thread object oop

Sure. The Thread::exit rather than JavaThread::exit was a typo.

Thanks,
David

> Thanks
> Markus
> 
> 
> -----Original Message-----
> From: David Holmes
> Sent: den 19 december 2018 22:22
> To: hotspot-dev developers <hotspot-dev at openjdk.java.net>; KIM.BARRETT <kim.barrett at oracle.com>; Markus Grönlund <markus.gronlund at oracle.com>
> Subject: RFR: JDK-8214097: Rework thread initialization and teardown logic
> 
> 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/
> 
> 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
>