Request for Review (XXL): 7104647: Adding a diagnostic command framework (original) (raw)

Frederic Parain frederic.parain at oracle.com
Wed Dec 7 21:30:51 UTC 2011

On 12/7/11 2:32 AM, Mandy Chung wrote:

On 12/2/2011 5:57 AM, Frederic Parain wrote:

http://cr.openjdk.java.net/~fparain/7104647/webrev.jdk.00/ L112: Since you are in this file, can you fix the typo "java.security.SecurityException" to "java.lang.SecurityException"?

Fixed.

The execute method doesn't throw any exception other than IAE. What happens if the specified command execution fails in the target VM? If no exception is thrown, how does the caller detect if the command succeeds or not and handle it gracefully? It seems that this mechanism should provide a way to detect if a command succeeds or fails programmatically rather than burying the error message in the returned string.

This changeset contains the framework, and the framework only throws NPE or IAE. However, a diagnostic command impementation is free to throw any other exception. If such an exception is thrown, it is simply printed on the output stream if the command has been invoked through the attachAPI, or it is propagated like any other exception through JMX if the command has been invoked from the HotSpotDiagnosticMXBean.

HotSpotDiagnostic.java L45-49: jvm is not used and can be removed.

Removed.

L133: do you need to check if command == null and throw NPE? or NPE will be thrown somewhere?

The native code is protected against null commands, it will throw the NPE.

L159: NPE should be thrown instead of IAE, right? Unless the javadoc for the execute method is modified to reflect that.

Fixed.

L176: minor: I wonder if it's any simpler for this native method to take the DiagnosticCommandInfo[] argument (i.e. the caller method does the array allocation than in the native method implementation).

It doesn't really help, because native code has allocations to performs anyway (for argument info).

ManagementFactoryHelper.java L270: The new parameter "jvm" doesn't seem to be needed.

Fixed.

jmm.h L316-326: the parameters should be lined up with the first one. L316-317 was aligned improperly and it's good to fix them in this batch.

Fixed

HotSpotDiagnostic.c L44, 51, 76-85: indentation needs fixing.

Fixed.

L158, 172: this should never happen unless the hotspot/jdk is mismatched. But would it better to throw an exception with an informative message to help diagnostic in case the mismatch does happen?

If a mismatch is detected, the implementation now throws an UnsupportedOperationException("Diagnostic commands are not supported by this VM").

jcmd.1 (man page) Should PerfCounter.print be listed in the man page? Or treat it like other diagnostic commands that are implementation-dependent?

PerfCounter is a built-in command, it doesn't appear in the list of commands returned by the 'help' command. This is why it is documented in the jcmd man page.

I'll upload new webrevs tomorrow morning.

Thanks,

Fred

-- Frederic Parain - Oracle Grenoble Engineering Center - France Phone: +33 4 76 18 81 17 Email: Frederic.Parain at oracle.com

More information about the core-libs-dev mailing list