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

[serguei.spitsyn at oracle.com]

Okay.

Thanks, Chris
Serguei

On 11/8/19 22:33, Chris Plummer wrote:
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>
>