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

[Daniel D. Daugherty]

On 11/14/18 7:30 AM, Markus Gronlund 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/

src/hotspot/share/jfr/jfr.cpp
     nit - L97 is a blank line at the end of the file; jcheck will
     likely complain.

src/hotspot/share/jfr/jfr.hpp
     No comments.

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp
     No comments.

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.hpp
     No comments.

src/hotspot/share/jfr/support/jfrThreadLocal.cpp
     No comments; you'll need someone more versed in JFR semantics
     to review this file's changes.

src/hotspot/share/jfr/support/jfrThreadLocal.hpp
     No comments.

src/hotspot/share/runtime/thread.cpp
     No comments.

Thumbs up!

Dan


>
> Thanks
> Markus
>
> -----Original Message-----
> From: Markus Gronlund
> Sent: den 14 november 2018 11:45
> To: David Holmes <david.holmes at oracle.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: RE: FW: RFR(S): 8210024: JFR calls virtual is_Java_thread from ~Thread()
>
> Thanks David,
>
> "is there any restriction/requirement/expectation that Jfr::on_thread_exit(t) is called from t? For JavaThreads it is always; for NonJavaThreads it may or may not."
>
> Great question.
>
> Presuppositions:
>
> For JavaThreads:        Thread::current() == t  // only because they also issue thread local events (ThreadEndEvent, ThreadCPULoadEvent) as part of exit.
> For NonJavaThreads: (Thread::current() == t || Thread::current() != t) // the buffer release code supports execution by a thread that is not the holder.
>
> I will add an assert in the JavaThread specific code sections of Jfr::on_thread_exit(t) , that Thread::current() == t.
>
> Thanks again for good input.
> Markus
>
>
> -----Original Message-----
> From: David Holmes
> Sent: den 14 november 2018 10:05
> To: Markus Gronlund <markus.gronlund at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: FW: RFR(S): 8210024: JFR calls virtual is_Java_thread from ~Thread()
>
> Hi Markus,
>
> The changes to the runtime code in thread.cpp seems fine.
>
> The Jfr changes I can't really speak to, but they seem reasonable. One
> question: is there any restriction/requirement/expectation that
> Jfr::on_thread_exit(t) is called from t? For JavaThreads it is always; for NonJavaThreads it may or may not.
>
> Thanks,
> David
> -----
>
> On 14/11/2018 6:50 pm, Markus Gronlund wrote:
>> Greetings,
>>
>> This RFR should have been sent to hotspot-runtime-dev as well as part of the original posting.
>>
>> Thank you
>> Markus
>>
>> -----Original Message-----
>> From: Markus Gronlund
>> Sent: den 12 november 2018 13:59
>> To: hotspot-jfr-dev at openjdk.java.net; Kim Barrett
>> <kim.barrett at oracle.com>
>> Subject: RFR(S): 8210024: JFR calls virtual is_Java_thread from
>> ~Thread()
>>
>> Greetings,
>>
>> Please review the following changes to help make the JFR deregistration / buffer release steps a bit more robust as thread exits.
>>
>> There is a need to release the JNIHandle for the EventWriter while the JavaThread is still on the threads list, since there is a sensitive interplay with the G1-prebarrier, and this change will help assist in the removal of shared PtrQueues.
>>
>> The virtual is_Java_Thread() call can still be invoked for the NonJavaThreads in their destructor, but since the thread is still a NonJavaThread at this point, it should be possible to make is_Java_thread() pure virtual in Thread.
>> I did not find any proper non-platform specific exit location for NonJavaThreads, so they release as part of the destructor.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210024
>> Webrev: http://cr.openjdk.java.net/~mgronlun/8210024/webrev01/
>> Testing: JFR tests, tier1, tier2, tier3
>>
>> Comments:
>> The JfrThreadSampler changes should have been done when the JfrThreadSampler thread was turned into a NonJavaThread - removing the dead export code as part of this change.
>> The Jfr::on_java_thread_dismantle() is an intermediary support that will be removed when we change overall thread registration from on_exit() to on_start(), which are planned upcoming changes.
>>
>> Thanks to Kim for detailing several involved issues.
>>
>> Thanks
>> Markus
>>