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