RFR(S): 8210024: JFR calls virtual is_Java_thread from ~Thread()

[Markus Gronlund]

Thanks David,

I tried for that location in my first try, but as it currently stands, and as the comment points out:

  // Note: at this point the thread object may already have deleted itself.
  // So from here on do not dereference *this*.

JavaThreads were already deallocated post "this->run()", so I did not even dare check the type of the thread there.

Great if you are doing things in this area, I can then follow-up on your work.

Cheers
Markus

-----Original Message-----
From: David Holmes 
Sent: den 16 november 2018 10:47
To: Markus Gronlund <markus.gronlund at oracle.com>; Kim Barrett <kim.barrett at oracle.com>
Cc: hotspot-runtime-dev at openjdk.java.net; hotspot-jfr-dev at openjdk.java.net
Subject: Re: RFR(S): 8210024: JFR calls virtual is_Java_thread from ~Thread()

Hi Markus,

We now have the shared non-virtual Thread::call_run method that can be used for common initialization before run(), and common tear-down after
run() (for threads that actually terminate). I'm looking at moving the NJT-list de-registration there due to a problem with terminating threads not actually getting deleted.

David

On 16/11/2018 7:36 pm, Markus Gronlund wrote:
> Hi Kim,
> 
> I agree it is not ideal to have a separate thread manipulating another threads "thread local", although the subsystem supports this in the case where they are not concurrent.
> 
> It was not built for this originally; originally it was built under the assumption that the threads themselves deregister via Jfr::on_thread_exit() before leaving the system, similar to what JavaThreads do inside JavaThread::exit().
> 
> However, as David pointed out, for NonJavaThreads this might not be the case.
> 
> I could not find an equivalent to JavaThread::exit() for NonJavaThreads that are non-platform specific, so the deregistration hook ended up in the NonJavaThread destructor.
> 
> If we can pinpoint a stable, non-platform specific exit location for NonJavaThreads, then this can be re-asserted to be thread local.
> Do you know of any such location?
> 
> Thanks
> Markus
> 
> 
> -----Original Message-----
> From: Kim Barrett
> Sent: den 16 november 2018 07:42
> To: Markus Gronlund <markus.gronlund at oracle.com>
> Cc: hotspot-runtime-dev at openjdk.java.net; 
> hotspot-jfr-dev at openjdk.java.net; David Holmes 
> <david.holmes at oracle.com>
> Subject: Re: RFR(S): 8210024: JFR calls virtual is_Java_thread from 
> ~Thread()
> 
>> On Nov 14, 2018, at 7:30 AM, Markus Gronlund <markus.gronlund at oracle.com> wrote:
>>
>> Hi again,
>>
>> Here is an updated webrev reflecting David's feedback.
>>
>> This feedback actually discovered a bug in the release code, for the situation where a thread running the release for another threads thread local buffer (a situation that could manifest with NonJavaThreads), would have asserted deeper in the release code, because it would have attempted to reserve a promotion buffer using "t" instead of itself. Subsequent asserts would then have trapped in the check for Thread::current() == t.
>> Thanks again David.
>>
>> The updated webrev takes care of this situation as well as making the presuppositions (stated below) more explicit using assertions.
>> Webrev02: http://cr.openjdk.java.net/~mgronlun/8210024/webrev02/
> 
> I found it very confusing to be passing around a JfrThreadLocal / Thread pair that are unrelated.  Are they really allowed to be so?  It looks like it, but that’s pretty hard to determine.  Maybe the parameters should have more descriptive names, like “current_thread” or something (rather than “t” or “thread”), and I’m not sure what for the JfrThreadLocal to indicate it need not be related to the thread.  Or maybe the current thread shouldn’t be passed through all these layers, but should just be obtained down at the leaf where it’s needed?
> 
>