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

[Chris Plummer]

Hi Serguei,

I've inlined below the corresponding code in HeapDumper::dump() that 
covers the deleted output functionality from attachListener.cpp. Note 
it's not exactly the same, but I think in the end it includes at least 
the same info in all cases, and in some cases more info:

-    int res = dumper.dump(op->arg(0));
-    if (res == 0) {
-      out->print_cr("Heap dump file created");
2009       out->print_cr("Heap dump file created [" JULONG_FORMAT " 
bytes in %3.3f secs]",
2010                     writer.bytes_written(), timer()->seconds());

-    } else {
-      // heap dump failed
-      ResourceMark rm;
-      char* error = dumper.error_as_C_string();
-      if (error == NULL) {
-        out->print_cr("Dump failed - reason unknown");
1986       out->print_cr("Unable to create %s: %s", path,
1987         (error() != NULL) ? error() : "reason unknown");

-      } else {
-        out->print_cr("%s", error);
-      }
2012       out->print_cr("Dump file is incomplete: %s", writer.error());

-    }
+    dumper.dump(op->arg(0), out);

Chris

On 11/8/19 6:31 PM, serguei.spitsyn at oracle.com wrote:
> 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
>>>>>
>>>>
>>>
>>
>