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

David Holmes david.holmes at oracle.com
Tue Nov 5 23:26:49 UTC 2019


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-November/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-November/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/webrev/
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> 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-runtime-dev mailing list