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

2019-11-11 Thread serguei.spit...@oracle.com
Hi Ralf, Okay, thanks! Serguei On 11/11/19 00:48, Schmelter, Ralf wrote: Hi Serguei, Thanks for the review. The only open question seems to be: The dumper.dump() returns int value. Returned value is not used anymore in the attachListener.cpp and diagnisticCommand.cpp. Is it still used

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

2019-11-11 Thread Schmelter, Ralf
Hi Serguei, Thanks for the review. The only open question seems to be: > 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? The jmm_DumpHeap0 method

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

2019-11-09 Thread serguei.spit...@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

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

2019-11-08 Thread 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

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

2019-11-08 Thread serguei.spit...@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

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

2019-11-08 Thread Chris Plummer
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

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

2019-11-08 Thread serguei.spit...@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

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

2019-11-08 Thread Chris Plummer
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.spit...@oracle.com wrote: Hi Ralf, The fix looks Okay in general. A couple of questions. The dumper.dump() returns int value. Returned value is

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

2019-11-08 Thread serguei.spit...@oracle.com
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

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

2019-11-08 Thread Thomas Stüfe
Hi Ralf, On Fri, Nov 8, 2019 at 4:45 PM Schmelter, Ralf wrote: > Hi Thomas, > > thanks for the review. > > > > Can you please add a comment about the new parameter? E.g. "optional > outputStream to which progress- and error messages will be written". > > Will do. > > > > - in

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

2019-11-08 Thread Schmelter, Ralf
Hi Thomas, thanks for the review. > Can you please add a comment about the new parameter? E.g. "optional > outputStream to which progress- and error messages will be written". Will do. > - in HeapDumper::dump(): while you are on it could you please initialize > my_path to NULL at function

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

2019-11-08 Thread Thomas Stüfe
Hi Ralf, this makes sense. Some small remarks: --- heapDumper.hpp // dumps the heap to the specified file, returns 0 if success. - int dump(const char* path); + int dump(const char* path, outputStream* out = NULL); Can you please add a comment about the new parameter? E.g. "optional

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

2019-11-08 Thread Schmelter, Ralf
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