PING: RFR: 8233706: JFR emergency dump should be performed after error reporting

Thu Mar 19 14:37:44 UTC 2020

On 2020/03/19 21:04, Markus Gronlund wrote:
> I don't understand it, what is the point of passing a static constant as an argument??

append() and get_current_directory() assume length of an argument (path) is JVM_MAXPATHLEN or longer, I think it is unsafe because shorten buffer might be added in future.
So I passed _path_buffer and size of it (_max_path_buffer_size) to some functions.

Yasumasa

> Markus
> 
> -----Original Message-----
> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
> Sent: den 19 mars 2020 03:53
> To: Markus Gronlund <markus.gronlund at oracle.com>
> Cc: hotspot-jfr-dev at openjdk.java.net; yasuenag at gmail.com
> Subject: PING: RFR: 8233706: JFR emergency dump should be performed after error reporting
> 
> Hi Markus,
> 
> What do you think about this change?
> 
>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8233706/webrev.01/
>>       difference from yours:
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233706/webrev.01/diff.patch
> 
> Yasumasa
> 
> 
> On 2020/03/03 15:28, Yasumasa Suenaga wrote:
>> Hi Markus,
>>
>> Thanks for sharing your change!
>> I tweaked it especially handling string length.
>>
>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8233706/webrev.01/
>>       difference from yours:
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233706/webrev.01/diff.patch
>>
>> On 2020/03/03 3:27, Markus Gronlund wrote:
>>> Hi Yasumasa,
>>>
>>> Apologies for a late response, I have been a bit swamped lately.
>>>
>>> Thank you for your work on trying to get information about JFR integrated with error reporting in Hotspot.
>>>
>>> This output is a bit ambiguous:
>>>
>>> # JFR recording file will be written. Location:
>>> # - /home/ysuenaga/hs_err_pid3734.jfr # -
>>> /tmp/2020_02_11_20_56_49_3734
>>>
>>> Does this mean that a recording file is written in two places (I know one path is the repository)?
>>
>> Yes, but I think your change is more suitable for this enhancement.
>>
>>
>>> I think it would be preferable to only print one line of information: The emergency dump file is a concatenation of the files in the repository. If it is successfully created, then references to the repository itself should not be needed. If emergency dump file creation fails, then perhaps, there could be value in printing the repository location. But then the output message would have to reflect that in some way (i.e. not a file, but a directory (a location), not created, but already existing...).
>>>
>>> Also, what if a non-attached thread issues this the to the hs_err<pid>.log file? It has now reported that a file will be written, but that thread will never reach the point of writing this file later?
>>
>> I made webrev.00 for this case to show repository path together, but I think your change is great for it.
>>
>>
>>> In addition, there seem to be no need to create the file descriptor early, since nothing is conditional on the outcome?
>>>
>>> I appreciate your concern and attempts to also have non-attached threads run the error reporting logic, by replacing the ResourceArea's with statically allocated buffers, although I think it can be streamlined a bit.
>>>
>>> One vision down this path you have initiated is that if we can make JfrEmergencyDump "non-attached thread friendly", together with the changes that were done as of JFR Event Streaming, then we are in a position that will not require the emergency dumper thread to issue invoke_flush() as part of on_vm_shutdown() (because an existing chunk is now already parsable). The emergency dumper thread would then only need to close the existing chunk file handle and issue the file copying operation. In order to have that possibility however, the rest of the ResourceAreas in JfrEmergencyDump (RepositoryIterator and write_emergency_file()) will also need to be removed / replaced. To bring everything home also the events must be conditional and the mutex protecting the chunkwriter will need to change to a lower level construct. Doable but requires some detailed planning.
>>
>> I think our goal of this enhancement is to give the location for emergency dump or repository path for postmortem analysis to user. Point of its view, your change is enough for it.
>>
>> If we encounter the problem which relates to ResourceAreas in future, I think it is not too late to start to work for it since then.
>>
>>
>>> I would like to suggest that we do the following changes (derived from your initial patches) to move closer to that position:
>>>
>>> Webrev: http://cr.openjdk.java.net/~mgronlun/8233706/webrev01/
>>>
>>> Note that this also prints JFR information (if applicable) to stdout making it behave very similar to core file reporting.
>>
>> I think it is very appreciate for container user!
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>> Thank you
>>> Markus
>>>
>>>
>>> -----Original Message-----
>>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>> Sent: den 24 februari 2020 14:58
>>> To: Markus Gronlund <markus.gronlund at oracle.com>;
>>> hotspot-jfr-dev at openjdk.java.net
>>> Cc: yasuenag at gmail.com
>>> Subject: PING: RFR: 8233706: JFR emergency dump should be performed
>>> after error reporting
>>>
>>> PING: Could you review it?
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8233706
>>>         https://bugs.openjdk.java.net/browse/JDK-8233373
>>>
>>> webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8233373/webrev.03/
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/02/11 22:46, Yasumasa Suenaga wrote:
>>>> Hi Markus,
>>>>
>>>> I uploaded new webrev which contains both 8233706 and 8233373.
>>>> Also it contains the fix which are commented by Thomas.
>>>>
>>>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8233373/webrev.03/
>>>>
>>>> This change passed all tests in submit repo (mach5-one-ysuenaga-JDK-8233373-3-20200211-1212-8648983).
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2020/02/10 21:12, Markus Gronlund wrote:
>>>>> Hi,
>>>>>
>>>>> I don't follow this change as it stands in isolation.
>>>>> Ok, there is a call to attempt to create the file descriptor early, but there is no state saved in relation to it, for example, the path constructed is not saved so it can be reported in the next step in the hs_err file? One has to question the reason for this early creation then. Is this the way you envision to accomplish this? To query later on the status of the FD to determine what path(s) to report during hs_err.log printing? And rely on the ability to reconstruct the same path again just in the next couple of instructions? If so, maybe create and save the actual path you want reported as part of the FD operation (success / failure) state?
>>>>>
>>>>> Can you provide a complete webrev with the entire solution (even if you have to bring in patches from two bugs), so this becomes self-contained. It is very hard to determine the correctness of this otherwise.
>>>>>
>>>>> Thanks
>>>>> Markus
>>>>>
>>>>> -----Original Message-----
>>>>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>>>> Sent: den 2 februari 2020 17:32
>>>>> To: hotspot-jfr-dev at openjdk.java.net
>>>>> Cc: Thomas Stüfe <thomas.stuefe at gmail.com>; Markus Gronlund
>>>>> <markus.gronlund at oracle.com>; yasuenag at gmail.com
>>>>> Subject: RFR: 8233706: JFR emergency dump should be performed after
>>>>> error reporting
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Please review this change.
>>>>>
>>>>>       JBS: https://bugs.openjdk.java.net/browse/JDK-8233706
>>>>>       webrev:
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233706/webrev.00/
>>>>>
>>>>> Error reporting (hs_err log) should happen as close as possible to the error point.
>>>>> However JFR emergency dump would be performed before it.
>>>>>
>>>>> JFR emergency dump should be performed after error reporting (e.g.
>>>>> NMT reporting, CI Replay)
>>>>>
>>>>> This webrev separates opening FD like minidump for Windows and writing flight record.
>>>>> Also create_emergency_dump_path() is no longer dependent on ResourceMark.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>