Fwd: RFR: 8233375: JFR emergency dump do not recover thread state
David Holmes
david.holmes at oracle.com
Mon Nov 4 02:24:55 UTC 2019
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/webrev.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/webrev.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