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

[David Holmes]

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

Sure -if we need some additional guarantees for the "shared post-run" 
then we can move things to meet those guarantees.

BTW I got tripped up by the gtest threads initially. AFAICS these 
"JavaThreads" don't have a Java threadObj! So care needs to be taken 
about assumptions during post-run.

Cheers,
David

> 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.
>>