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.spit...@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.spit...@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.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 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
