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

[Markus Gronlund]

Thanks David for doing this work.

Looks good.

"
- 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");

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

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