Fwd: RFR: 8233375: JFR emergency dump do not recover thread state

Yasumasa Suenaga suenaga at oss.nttdata.com
Wed Nov 6 12:25:50 UTC 2019 

Thanks Markus!

Yasumasa

On 2019/11/06 18:17, Markus Gronlund wrote:
> Looks good.
> 
> Markus
> 
> -----Original Message-----
> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
> Sent: den 6 november 2019 00:40
> To: David Holmes <david.holmes at oracle.com>; Markus Gronlund <markus.gronlund at oracle.com>
> Cc: hotspot-jfr-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; yasuenag at gmail.com
> Subject: Re: Fwd: RFR: 8233375: JFR emergency dump do not recover thread state
> 
> Thanks David!
> Markus, please review it:
> 
>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8233375/webrev.05/
> 
> 
> Yasumasa
> 
> 
> On 2019/11/06 8:26, David Holmes wrote:
>> On 5/11/2019 11:40 pm, Yasumasa Suenaga wrote:
>>> On 2019/11/05 22:34, Yasumasa Suenaga wrote:
>>>> Hi Markus,
>>>>
>>>> Thanks for explanation. I tweaked your webrev:
>>>>
>>>>     http://cr.openjdk.java.net/~mgronlun/8233375/webrev01/
>>>
>>> Sorry, wevrev is here:
>>>
>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8233375/webrev.05/
>>
>> This seems okay to me.
>>
>> Thanks,
>> David
>>
>>> Yasumasa
>>>
>>>> If you and David are ok, I will push it.
>>>>
>>>>
>>>>> The reason is that Event Streaming changed the architecture a bit where existing data will move to the chunk file more frequently (including artifacts) and it might therefore be possible to create a non-VM internal dump mechanism (bounding it to mainly closing the underlying file and copying chunks).
>>>>
>>>> I think it is very helpful for troubleshooting!
>>>> If crash report shows the location of JFR file or repository, it is more useful.
>>>> So I've sent review request for it (JDK-8233373):
>>>>
>>>> https://mail.openjdk.java.net/pipermail/hotspot-jfr-dev/2019-Novembe
>>>> r/000808.html
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2019/11/05 21:54, Markus Gronlund wrote:
>>>>> The current dump mechanism is reusing most of the regular logic because it has to perform quite a lot of work to construct a recording. For example, it needs to collect all tagged artifacts in the system (Klass, Method, ClassLoaderData, Symbols, Modules and more) to have the events in an emergency recording file be fully parsable. This is non-trivial requiring a thread to at least be part of the VM, with most thread local data structures preserved.
>>>>>
>>>>> We should remember that dumping an emergency recording is only a best effort attempt. As an analogy, compare it to other routines in VMError::report() that are conditional and require a non-NULL thread object (print the current Compile Task, print VM Operation or event printing the JavaStack for a thread for example).
>>>>>
>>>>> With Event Streaming that was recently checked in, it is easier (although not easy, but easier compared to before ) to extend this support to not have the emergency dumper thread do so much internal VM work.
>>>>> The reason is that Event Streaming changed the architecture a bit where existing data will move to the chunk file more frequently (including artifacts) and it might therefore be possible to create a non-VM internal dump mechanism (bounding it to mainly closing the underlying file and copying chunks).
>>>>>
>>>>> It is unclear at this point if the value-add is high enough to warrant the work.
>>>>>
>>>>> Thanks
>>>>> Markus
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>>>> Sent: den 5 november 2019 12:56
>>>>> To: Markus Gronlund <markus.gronlund at oracle.com>; David Holmes
>>>>> <david.holmes at oracle.com>
>>>>> Cc: hotspot-jfr-dev at openjdk.java.net;
>>>>> hotspot-runtime-dev at openjdk.java.net; yasuenag at gmail.com
>>>>> Subject: Re: Fwd: RFR: 8233375: JFR emergency dump do not recover
>>>>> thread state
>>>>>
>>>>> Hi Markus,
>>>>>
>>>>>> 1. An invalid thread local will give immediate problems downstream, for example see JfrRecorderService::prepare_for_vm_error_rotation().
>>>>>
>>>>> Do you mean that it would not perform when Thread::current() returns NULL?
>>>>> It will happen when crash is occur in detached thread [1]. Can't we think about that case?
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> [1]
>>>>> https://mail.openjdk.java.net/pipermail/hotspot-jfr-dev/2019-Novemb
>>>>> er/000822.html
>>>>>
>>>>>
>>>>> On 2019/11/05 19:35, Markus Gronlund wrote:
>>>>>> Hi again,
>>>>>>
>>>>>> The comments in my previous email still apply:
>>>>>>
>>>>>> "...although very unlikely, we could have run into an issue with thread local storage, so it makes sense to test this up front. If we cannot read the thread local, the operations we intend to perform will fail, so we might just bail out already. I took the liberty to tighten up the transition class a little bit; you only need to restore the thread state if there was an actual change."
>>>>>>
>>>>>> 1. An invalid thread local will give immediate problems downstream, for example see JfrRecorderService::prepare_for_vm_error_rotation().
>>>>>> 2. You don't need to restore _thread_in_vm back to threads already running in the correct state. The purpose of the transition helper class is to move Java threads not running in _thread_in_vm (i.e. will be _thread_in_native). Move this logic to the fore to better clarify the intent of the helper class.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mgronlun/8233375/webrev01/
>>>>>>
>>>>>> Thanks
>>>>>> Markus
>>>>>>
>>>>>>
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>>>>> Sent: den 5 november 2019 07:36
>>>>>> To: David Holmes <david.holmes at oracle.com>; Markus Gronlund
>>>>>> <markus.gronlund at oracle.com>
>>>>>> Cc: hotspot-jfr-dev at openjdk.java.net;
>>>>>> hotspot-runtime-dev at openjdk.java.net; yasuenag at gmail.com
>>>>>> Subject: Re: Fwd: RFR: 8233375: JFR emergency dump do not recover
>>>>>> thread state
>>>>>>
>>>>>> Thanks David!
>>>>>> I wait for Markus's review.
>>>>>>
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2019/11/05 15:19, David Holmes wrote:
>>>>>>> On 5/11/2019 4:08 pm, Yasumasa Suenaga wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> Sorry, I was confused :)
>>>>>>>> This is new webrev. Could you check again?
>>>>>>>>
>>>>>>>>       http://cr.openjdk.java.net/~ysuenaga/JDK-8233375/webrev.04/
>>>>>>>
>>>>>>> Okay structurally I'm fine with that.
>>>>>>>
>>>>>>> What I don't know, and will leave to Markus to determine, is whether the rest of the code in on_vm_shutdown can actually execute okay if there is no current thread.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2019/11/05 14:56, David Holmes wrote:
>>>>>>>>> On 5/11/2019 3:48 pm, Yasumasa Suenaga wrote:
>>>>>>>>>> On 2019/11/05 14:34, David Holmes wrote:
>>>>>>>>>>> On 5/11/2019 3:13 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>
>>>>>>>>>>>>> It would be cleaner/simpler if prepare_for_emergency_dump takes the thread argument. As it is just a static function that doesn't impact anything else.
>>>>>>>>>>>>
>>>>>>>>>>>> prepare_for_emergency_dump() returns false if some critical locks could not unlock.
>>>>>>>>>>>> So what should we return if NULL is passed as the argument? true?
>>>>>>>>>>>
>>>>>>>>>>> But you're not calling prepare_for_emergency_dump when the thread is NULL:
>>>>>>>>>>>
>>>>>>>>>>>      454   Thread* thread = Thread::current_or_null_safe();
>>>>>>>>>>>      455
>>>>>>>>>>>      456   // Ensure a JavaThread is _thread_in_vm when we
>>>>>>>>>>> make this call
>>>>>>>>>>>      457   JavaThreadInVM jtivm(thread);
>>>>>>>>>>>      458   if ((thread != NULL) &&
>>>>>>>>>>> !prepare_for_emergency_dump()) {
>>>>>>>>>>>      459     return;
>>>>>>>>>>>      460   }
>>>>>>>>>>
>>>>>>>>>> Oh, sorry, I have a mistake!
>>>>>>>>>> I want to change as below:
>>>>>>>>>>
>>>>>>>>>> ```
>>>>>>>>>> +  Thread* thread = Thread::current_or_null_safe();
>>>>>>>>>> +
>>>>>>>>>> +  // Ensure a JavaThread is _thread_in_vm when we make this
>>>>>>>>>> +call
>>>>>>>>>> +  JavaThreadInVM jtivm(thread);
>>>>>>>>>> +  if (thread != NULL) {
>>>>>>>>>> +    if (!prepare_for_emergency_dump()) {
>>>>>>>>>> +      return;
>>>>>>>>>> +    }
>>>>>>>>>> +  }
>>>>>>>>>> ```
>>>>>>>>>
>>>>>>>>> but that is the same logic ??
>>>>>>>>>
>>>>>>>>>>> All I'm saying is that you pass "thread" as a parameter so you can then delete the existing call to Thread::current() that is inside prepare_for_emergency_dump.
>>>>>>>>>>
>>>>>>>>>> Is it prefer pass "thread" to prepare_for_emergency_dump() instead of above?
>>>>>>>>>
>>>>>>>>> ??? The two are not related. If you've already obtained the current thread you can pass it to prepare_for_emergency_dump and avoid the need to call Thread:current() (in whatever form) again. How you handle a NULL current thread is independent of that.
>>>>>>>>>
>>>>>>>>>> If so, I will push new changeset to submit repo, and will send new review request.
>>>>>>>>>
>>>>>>>>> I'd send the review request first and get agreement before wasting time on the submit repo.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> David
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>>> It might break semantics of this function, so I did not add argument to prepare_for_emergency_dump() in this webrev.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2019/11/05 13:56, David Holmes wrote:
>>>>>>>>>>>>> On 5/11/2019 1:56 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>> On 2019/11/05 9:17, David Holmes wrote:
>>>>>>>>>>>>>>> On 5/11/2019 9:56 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>> On 2019/11/04 22:43, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> Hi Markus,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I thought similar change, and it is running on submit repo:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>       
>>>>>>>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/76703c4ec1ea
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> If it passes all tests, I will send review request again.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This change passed all tests on submit repo (mach5-one-ysuenaga-JDK-8233375-2-20191104-1317-6405064).
>>>>>>>>>>>>>>>> Could you review again?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233375/webrev.
>>>>>>>>>>>>>>>> 02/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In Markus's change, emergency dump will not perform when Thread::current_or_null_safe() returns NULL.
>>>>>>>>>>>>>>>> However it will occur when coredump signal (e.g. SIGSEGV) throws to PID by `kill` command - main thread of the process will be already detached (out of JVM).
>>>>>>>>>>>>>>>> Also the crash might happen in native thread - created by pthread_create (on Linux) from JNI code.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thus we should continue to perform emergency dump even if Thread::current_or_null_safe() returns NULL.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I didn't quite follow all that, but if there is no current thread then prepare_for_emergency_dump() is either going to assert here:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>      348   Thread* const thread = Thread::current();
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> or crash here:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>      349   if (thread->is_Watcher_thread()) {
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks David!
>>>>>>>>>>>>>> I fixed it in new webrev:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233375/webrev.03
>>>>>>>>>>>>>> /
>>>>>>>>>>>>>
>>>>>>>>>>>>> It would be cleaner/simpler if prepare_for_emergency_dump takes the thread argument. As it is just a static function that doesn't impact anything else.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> David
>>>>>>>>>>>>>
>>>>>>>>>>>>>> It works fine on submit repo (mach5-one-ysuenaga-JDK-8233375-2-20191105-0117-6422777).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 2019/11/04 22:23, Markus Gronlund wrote:
>>>>>>>>>>>>>>>>>> Hi Yasumasa and David,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Apologies about the suggestion to use ThreadInVMFromUnknown. I realized later that, as you have pointed out, it would perform a real thread transition. Sorry.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Taking some input from ThreadInVMFromUnknown and the situations I have seen at this location, I think the only case we need to be concerned about here is when a JavaThread is _thread_in_native. _thread_in_java transition to _thread_in_vm via stubs in SharedRuntime (i believe) as part of coming out of the exception handler(s). Unfortunately I cannot give a proper argument now to give the premises where this invariant is enforced, so let's work with the original thread state as you suggested Yasumasa.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> If we can avoid passing the thread all the way through, I think that is preferable (this is not performance critical code). David also alluded to the fact that you always manipulate the current thread anyway. Although very unlikely, we could have run into an issue with thread local storage, so it makes sense to test this up front. If we cannot read the thread local, the operations we intend to perform will fail, so we might just bail out already.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I took the liberty to tighten up the transition class a little bit; you only need to restore the thread state if there was an actual change.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Perhaps we can do it like this?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~mgronlun/8233375/webrev01/
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks for your patience investigating this
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Markus
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>>>>> From: David Holmes
>>>>>>>>>>>>>>>>>> Sent: den 4 november 2019 05:24
>>>>>>>>>>>>>>>>>> To: Yasumasa Suenaga <suenaga at oss.nttdata.com>; Markus
>>>>>>>>>>>>>>>>>> Gronlund <markus.gronlund at oracle.com>;
>>>>>>>>>>>>>>>>>> hotspot-jfr-dev at openjdk.java.net;
>>>>>>>>>>>>>>>>>> hotspot-runtime-dev at openjdk.java.net
>>>>>>>>>>>>>>>>>> Cc: yasuenag at gmail.com
>>>>>>>>>>>>>>>>>> Subject: Re: Fwd: RFR: 8233375: JFR emergency dump do
>>>>>>>>>>>>>>>>>> not recover thread state
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So looking at Yasumasa's proposed fix ...
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I don't think it is worth the disruption to pass the
>>>>>>>>>>>>>>>>>> "thread" all the way through these API's. It is
>>>>>>>>>>>>>>>>>> simpler/cleaner to just call
>>>>>>>>>>>>>>>>>> Thread::current_or_null_safe() when you need the current thread.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 357   assert(thread->is_Java_thread() &&
>>>>>>>>>>>>>>>>>> (((JavaThread*)thread)->thread_state() ==
>>>>>>>>>>>>>>>>>> _thread_in_vm), "invariant");
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> This assertion is incorrect. As this can be called via
>>>>>>>>>>>>>>>>>> VMError::report_or_die() there is AFAICS absolutely no guarantee that we need be in a JavaThread at all.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>       428 class ThreadInVMForJFR : public StackObj {
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Can I suggest JavaThreadInVM to make it clear this only affects JavaThreads. And as it is local we don't need the "forJFR" part.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Based on Markus's proposed change, and with a view to constrain the scope even further can I suggest the following:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> if (!guard_reentrancy()) {
>>>>>>>>>>>>>>>>>>        return;
>>>>>>>>>>>>>>>>>> } else {
>>>>>>>>>>>>>>>>>>        // Ensure a JavaThread is _thread_in_vm when we
>>>>>>>>>>>>>>>>>> make this call
>>>>>>>>>>>>>>>>>>        JavaThreadInVM
>>>>>>>>>>>>>>>>>> jtivm(Thread::current_or_null_safe());
>>>>>>>>>>>>>>>>>>        if (!prepare_for_emergency_dump()) {
>>>>>>>>>>>>>>>>>>          return;
>>>>>>>>>>>>>>>>>>        }
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 4/11/2019 12:24 pm, David Holmes wrote:
>>>>>>>>>>>>>>>>>>> Correction ...
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 4/11/2019 12:11 pm, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>> On 4/11/2019 11:19 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>> On 2019/11/04 7:38, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>>> On 4/11/2019 2:22 am, Markus Gronlund wrote:
>>>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> I think you can simplify it to something like this:
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~mgronlun/8233375/webr
>>>>>>>>>>>>>>>>>>>>>>> ev/
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> That is more like I had envisaged for this.
>>>>>>>>>>>>>>>>>>>>>> Reusing existing thread-state transition code is
>>>>>>>>>>>>>>>>>>>>>> preferable to adding more custom code that directly manipulates thread-state.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> I do not agree with this change.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> VMError::report_and_die() has "Thread* thread" in
>>>>>>>>>>>>>>>>>>>>> its arguments. So
>>>>>>>>>>>>>>>>>>>>> Thread::current() might be different with it.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Not sure what you mean. You only ever manipulate the
>>>>>>>>>>>>>>>>>>>> thread state of the current thread.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> In addition, ThreadInVMfromUnknown uses
>>>>>>>>>>>>>>>>>>>>> transition_from_native() to change the thread state.
>>>>>>>>>>>>>>>>>>>>> It checks (and manipulates?) something which relates to safepoint.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Yes it does - which would be a problem if a
>>>>>>>>>>>>>>>>>>>> safepoint (or
>>>>>>>>>>>>>>>>>>>> handshake) were pending. But the path through
>>>>>>>>>>>>>>>>>>>> before_exit already has safepoint checks when you acquire the BeforeExit_lock.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> But that isn't relevant. The issue is we don't want a
>>>>>>>>>>>>>>>>>>> safepoint check on the report_and_die() path. So a
>>>>>>>>>>>>>>>>>>> custom transition helper is needed to avoid that.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> The main problem with the suggestion is it seems we
>>>>>>>>>>>>>>>>>>>> may not be running in a JavaThread:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>       349   Thread* const thread = Thread::current();
>>>>>>>>>>>>>>>>>>>>       350   if (thread->is_Watcher_thread()) {
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> so we can't use the existing thread-state helpers,
>>>>>>>>>>>>>>>>>>>> unless we narrow the scope (as you do) to after the check for the WatcherThread.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Thus I added ThreadInVMForJFR to new my webrev.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Your change still seems overly complicated.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>>>>>>> Markus
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>>>>>>>>>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>>>>>>>>>>>>>>>>>>>>>> Sent: den 2 november 2019 16:57
>>>>>>>>>>>>>>>>>>>>>>> To: hotspot-jfr-dev at openjdk.java.net;
>>>>>>>>>>>>>>>>>>>>>>> hotspot-runtime-dev at openjdk.java.net
>>>>>>>>>>>>>>>>>>>>>>> Cc: David Holmes <david.holmes at oracle.com>;
>>>>>>>>>>>>>>>>>>>>>>> yasuenag at gmail.com; Markus Gronlund
>>>>>>>>>>>>>>>>>>>>>>> <markus.gronlund at oracle.com>
>>>>>>>>>>>>>>>>>>>>>>> Subject: Re: Fwd: RFR: 8233375: JFR emergency
>>>>>>>>>>>>>>>>>>>>>>> dump do not recover thread state
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Markus commented in JBS this change should be kept local to JFR.
>>>>>>>>>>>>>>>>>>>>>>> So I updated webrev. Could you review it?
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233375/
>>>>>>>>>>>>>>>>>>>>>>> webr
>>>>>>>>>>>>>>>>>>>>>>> e
>>>>>>>>>>>>>>>>>>>>>>> v.01/
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> This change passed all tests on submit repo
>>>>>>>>>>>>>>>>>>>>>>> (mach5-one-ysuenaga-JDK-8233375-1-20191102-1354-6373703).
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> On 2019/11/01 18:41, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>> Forward to hotspot-runtime-dev.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> As David commented in JBS, it may need to be fixed in JFR code.
>>>>>>>>>>>>>>>>>>>>>>>> But I'm not unclear why thread state is not recover.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> I'd like to hear about this from JFR folks.
>>>>>>>>>>>>>>>>>>>>>>>> If it is just a bug in JFR, I will create a
>>>>>>>>>>>>>>>>>>>>>>>> patch which recover it in JFR code.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> -------- Forwarded Message --------
>>>>>>>>>>>>>>>>>>>>>>>> Subject: RFR: 8233375: JFR emergency dump do not
>>>>>>>>>>>>>>>>>>>>>>>> recover thread state
>>>>>>>>>>>>>>>>>>>>>>>> Date: Fri, 1 Nov 2019 17:08:42 +0900
>>>>>>>>>>>>>>>>>>>>>>>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>>>>>>>>>>>>>>>>>>>>>>> To: hotspot-jfr-dev at openjdk.java.net
>>>>>>>>>>>>>>>>>>>>>>>> CC: yasuenag at gmail.com <yasuenag at gmail.com>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Please review this change:
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>         JBS:
>>>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8233375
>>>>>>>>>>>>>>>>>>>>>>>>         webrev:
>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233375
>>>>>>>>>>>>>>>>>>>>>>>> /web
>>>>>>>>>>>>>>>>>>>>>>>> r
>>>>>>>>>>>>>>>>>>>>>>>> ev.00/
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> If JFR is running when JVM crashes, JFR will
>>>>>>>>>>>>>>>>>>>>>>>> dump data to hs_err_pid<PID>.jfr .
>>>>>>>>>>>>>>>>>>>>>>>> It would perform in prepare_for_emergency_dump().
>>>>>>>>>>>>>>>>>>>>>>>> However this function transits thread state to "_thread_in_vm".
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> This change has been tested on submit repo as
>>>>>>>>>>>>>>>>>>>>>>>> mach5-one-ysuenaga-JDK-8233375-20191101-0651-6334762.
>>>>>>>>>>>>>>>>>>>>>>>> It failed at
>>>>>>>>>>>>>>>>>>>>>>>> compiler/types/correctness/CorrectnessTest.java
>>>>>>>>>>>>>>>>>>>>>>>> However this test is for JIT compiler, and
>>>>>>>>>>>>>>>>>>>>>>>> related issue has been reported as JDK-8225620.
>>>>>>>>>>>>>>>>>>>>>>>> So I think this patch can go through.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa

More information about the hotspot-jfr-dev mailing list