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

[serguei.spitsyn at oracle.com]

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
>>
>