RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap

[serguei.spitsyn at oracle.com]

Okay, thanks.
Agreed, this aspect was clear to me.
The deleted fragments are about printing the summarizing conclusion 
about the dumping which is not present in the HeapDumper::dump().
I expected it to be moved into the HeapDumper::dump() but it was not.
So, I wonder if there was such an intention.

Thanks,
Serguei


On 11/8/19 18:14, Chris Plummer wrote:
> He also added the outputStream argument to HeapDumper::dump(). Both of 
> the sections of code below already called dump(), and now they do so 
> with the added outputStream argument. HeapDumper::dump() has been 
> modified to print on the outputStream rather than to the tty.
>
> Chris
>
> On 11/8/19 5:02 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Chris,
>>
>> I'm a little bit confused.
>> The Ralf's change in the HeapDumper::dump() is just replacement of 
>> 'tty' occurrences with 'out',
>> so the change has not moved the deleted code in these files into the 
>> HeapDumper::dump().
>>
>> Probably, you wanted to say that the pre-existed error messages 
>> printed in the HeapDumper::dump() are enough.
>> This would explain why the code is deleted.
>> Just wanted a bit of clarification from Ralf to make sure it is the 
>> case.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 11/8/19 16:42, Chris Plummer wrote:
>>> Hi Ralf,
>>>
>>> Also looks good to me.
>>>
>>> Serguei, the removed code is consolidated into HeapDumper::dump().
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 11/8/19 4:25 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Ralf,
>>>>
>>>> The fix looks Okay in general.
>>>>
>>>>
>>>> A couple of questions.
>>>>
>>>> The dumper.dump() returns int value.
>>>> Returned value is not used anymore in the attachListener.cpp and 
>>>> diagnisticCommand.cpp.
>>>> Is it still used somewhere else or we can replace it with void?
>>>>
>>>> Could you explain a little bit why the following fragments were 
>>>> removed?
>>>> Is it because this information is not that useful or there is some 
>>>> other motivation?
>>>>
>>>> attachListener.cpp:
>>>> - int res = dumper.dump(op->arg(0));
>>>> - if (res == 0) {
>>>> - out->print_cr("Heap dump file created");
>>>> - } else {
>>>> - // heap dump failed
>>>> - ResourceMark rm;
>>>> - char* error = dumper.error_as_C_string();
>>>> - if (error == NULL) {
>>>> - out->print_cr("Dump failed - reason unknown");
>>>> - } else {
>>>> - out->print_cr("%s", error);
>>>> - }
>>>> - }
>>>>
>>>> diagnisticCommand.cpp
>>>> - if (res == 0) {
>>>> - output()->print_cr("Heap dump file created");
>>>> - } else {
>>>> - // heap dump failed
>>>> - ResourceMark rm;
>>>> - char* error = dumper.error_as_C_string();
>>>> - if (error == NULL) {
>>>> - output()->print_cr("Dump failed - reason unknown");
>>>> - } else {
>>>> - output()->print_cr("%s", error);
>>>> - }
>>>> - }
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 11/8/19 03:56, Schmelter, Ralf wrote:
>>>>> This change forwards the output from the HeapDumper.dump() command 
>>>>> to an optional output stream supplied by the caller.
>>>>> Until now the diagnositic command and the "dumpheap" command of 
>>>>> the attach framework partly recreated the output by hand, but 
>>>>> missing some information.
>>>>>
>>>>> Old output:
>>>>> Heap dump file created
>>>>>
>>>>> New output:
>>>>> Dumping heap to test.hprof ...
>>>>> Heap dump file created [9719330384 bytes in 27,759 secs]
>>>>>
>>>>> In addition to getting this improved information, it saves code too.
>>>>>
>>>>> Bugreport:https://bugs.openjdk.java.net/browse/JDK-8233790 
>>>>> Webrev:http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/ 
>>>>>
>>>>> Best regards,
>>>>> Ralf
>>>>
>>>
>>
>