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

[Markus Gronlund]

Understood.

The reason for suggesting that move is so that "shared post-run" can work with proper JavaThreads (not having oops for threadObj and threadGroup disappearing from underneath). 

We can take a closer look later.

Cheers
Markus

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

Hi Markus,

Glad to hear this may be of use - thanks for taking a look.

With regard to thread->exit(false) placement, it seems somewhat arbitrary as to whether something goes at the end of run() or the beginning of post_run(). So unless there was a strong reason to move something (like no delete before post_run()) I left things as they are.

Cheers,
David

On 11/12/2018 12:49 am, Markus Gronlund wrote:
> 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.
>