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