Request for Review: 7120511: Add diagnostic commands
frederic.parain at oracle.com
Fri Jan 6 04:15:29 PST 2012
Thanks for the review, my comments are inlined below.
On 01/ 5/12 05:19 PM, Karen Kinnear wrote:
> Code looks good.
> A couple of minor comments/questions:
> 1) diagnosticCommand.cpp HeapDumpDCmd::execute
> - There is a comment in attachListener under dump_heap that would
> be helpful to have here as well: // Request a full GC before heap dump if live_objects ...
A comment has been added in diagnosticCommand.cpp and
the help message of the HeapDumpDCmd has been updated too.
> 2) I wonder if it would be helpful for the new jcmds that share functionality with existing mechanisms
> if it is worth a comment in the code noting where else this logic is called - might
> help future bug fixers do thorough testing.
I've added cross-references in comments between the attachListener
file and the diagnosticCommand file.
> 3) attachListener jcmd
> You've added an out->cr() after throwing the exception here, but I wonder if
> you actually want it whether or not there was an exception thrown here -
> e.g. inside execute I see a number of places where there are print calls for
> exceptions and raw_print called - do those want the<cr> as well?
> Or are there cases where there is nothing printed so you would not want that? e.g. runFinalization?
The out->cr() in attachListener was correct, but other locations were
an exception was printed were lacking an out->cr() call. It's fixed now.
New webrev: http://cr.openjdk.java.net/~fparain/7120511/webrev.01/
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at Oracle.com
More information about the serviceability-dev