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

[Markus Gronlund]

Hi David,

Thank you for looking into this.

I think this approach is in the right direction. Specially as it gives a form and a vocabulary to reason about thread initialization and teardown states.

I like the fact you provide both shared as well as thread-specific locations for transitions, like:

Thread::call_run()

// shared pre-run
// specific pre-run
// shared run
// specific run
// shared post-run
// specific post-run

This means we can talk about this better now. 

I can for example say, "the hook is thread generic so it can go into "shared post-run"".
 
In addition, we can start to bind properties to them, much like you have done with the assertions for thread locals. 
It will help a lot to have the guarantee using pre-and post-conditions that a thread will not be deleted somewhere deeper in the call stack.

I could easily consolidate  JFR_ONLY(Jfr::on_thread_exit();), originally located in JavaThread::exit() and NonJavaThread::post_run() into "shared post-run".

I also elaborated with moving "this->exit(false);" from JavaThread::thread_main_inner() to JavaThread::post_run(), and that seemed to work quite well, although I understand that we are not moving all components at once.

The most important thing is that we get a protocol and locations in place for where things could be moved (which is what you are addressing).

Thanks again for doing this
Markus


-----Original Message-----
From: David Holmes 
Sent: den 10 december 2018 08:08
To: hotspot-dev developers <hotspot-dev at openjdk.java.net>
Cc: Kim Barrett <kim.barrett at oracle.com>; Markus Grönlund <markus.gronlund at oracle.com>
Subject: Prelim RFR: JDK-8214097: Rework thread initialization and teardown logic

This is a preliminary RFR to get feedback. I won't be pushing until 13 and the final RFR will have to sync with other changes that may impact what I'm doing here - e.g. Shenandoah GC.

@Markus: does this help with your JFR event hooks?

Bug: https://bugs.openjdk.java.net/browse/JDK-8214097
Webrev: http://cr.openjdk.java.net/~dholmes/8214097/webrev/

The primary change is that we introduce initialization and tear-down hooks for the different thread types so that we can consistently apply common actions - like adding/removing NJTs to the NJT::list - and ensure consistent lifecycle management. This is done by introducing virtual
pre_run() and post_run() hooks so that we arrange the non-virtual
Thread::call_run() so that it does:

void Thread::call_run() {

   // Perform common initialization actions

   register_thread_stack_with_NMT();
    ...

   // Perform <ChildClass> initialization actions
   this->pre_run();

   // Invoke <ChildClass>::run()
   this->run();

   // Perform common tear-down actions

   // Perform <ChildClass> tear-down actions
   this->post_run();

}

And establish some basic lifecycle rules, such as:
- no deletion of the thread is allowed prior to post_run()
- after post_run() 'this' should not be referenced
- Thread::current must be cleared before Thread deletion

I refactored a little bit of common setup by getting rid of NamedThread::initialize_named_thread(), and having NJT::pre_run() set the name.**

Added some comments about how the different thread entry points and hooks connect together.

Clarified some initialization related interactions between thread creation and BarrierSet creation (which Kim is trying to clean up under 8215097).

Reverted the change in management.cpp put in under JDK-8212207 as it is no longer needed now that NJTs only get added to the list in pre_run() and so must be valid. And somewhat related I added an assert on Linux that osthread->pthread_id() has been set before the call to call_run().

Simplified gtest/threadHelper.inline.hpp as the hooks introduced are now supplied by Thread.

Thanks to Kim Barrett for the updated NJT list management code and discussion on different approaches.

Thanks,
David

** The role of NamedThread seems quite tenous now, but it's still being used to store "stuff" that seems unrelated to being a NamedThread. 
Anyway that would be a future cleanup RFE.