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

David Holmes david.holmes at
Tue Dec 13 06:49:23 UTC 2011

Hi Fred,

A couple of minor comments on the JDK side:

Sorry if this is old ground but a query on the API design: is there a 
specific reason to use Lists rather than the arrays the VM will provide?

  139     public List<DiagnosticCommandInfo> getDiagnosticCommandInfo(
  140         List<String> commands) {
  141         if (commands == null) {
  142             throw new NullPointerException();
  143         }
  144         return Arrays.asList(getDiagnosticCommandInfo0(
  145             commands.toArray(new String[commands.size()])));
  146     }

The explicit null check is redundant as commands.size() will be the 
first thing invoked.



The structs should use an indent of 2 to match the style of the file.



  39 Java_sun_management_HotSpotDiagnostic_getDiagnosticCommands0
  40   (JNIEnv *env, jobject dummy)

Put on one line.

42   if((jmm_version > JMM_VERSION_1_2_1)

Space after 'if'

  50 jobject getDiagnosticCommandArgumentInfoArray(JNIEnv *env, jstring 
  51                                                    int num_arg) {

Align arg with opening parentheses

52-59, 107-112
It is not necessary with modern C compilers to declare all variables at 
the start of a block/function. You can declare them in the scope they 
will be used.

  61   dcmd_arg_info_array = (dcmdArgInfo*) malloc(num_arg * 

Cast is not needed.


General: I suggest generating the javadoc for your new classes and 
having an editorial check done. I spotted a couple of typos eg:
  32  * of one parameter of diagnostic command.
  89      * Returns the a list of the
                     ^^^^ typo


On 13/12/2011 12:31 AM, Frederic Parain wrote:
> Hi Robert,
> These issues should be solved with the new webrevs:
> Regards,
> Fred
> On 12/ 9/11 03:17 PM, Robert Ottenhag wrote:
>> Adding to the review of jmm.h, that is modified in both the jdk part
>> and the hotspot part, these files should be identical, without any
>> differences in whitespace.
>> Also, regarding whitespace and indentation, the use TAB characters in
>> many files in the jdk patch makes jcheck complain (when trying to
>> import your patch locally), and should be replaced by spaces.
>> Best regards,
>> /Robert
>>> -----Original Message----- From: Paul Hohensee Sent: Thursday,
>>> December 08, 2011 7:26 PM To: Frederic Parain Cc:
>>> serviceability-dev at;
>>> core-libs-dev at;
>>> hotspot-runtime-dev at Subject: Re: Request for
>>> Review (XXL): 7104647: Adding a diagnostic command framework
>>> For the hotspot part at
>>> Most of my comments are style-related. Nice job on the
>>> implementation architecture.
>>> In attachListener.cpp:
>>> You might want to delete the first "return JNI_OK;" line, since the
>>> code under HAS_PENDING_EXCEPTION just falls through.
>>> In jmm.h:
>>> Be nice to indent "(JNIEnv" on lines 318, 319 and 325 the same as
>>> the existing declarations. Add a newline just before it on line
>>> 322.
>>> In diagnosticFramework.hpp:
>>> Fix indenting for lines 63-66, insert blank before "size_t" on line
>>> 48.
>>> Hotspot convention is that getter method names don't include a
>>> "get_" prefix. So, e.g., DCmdArgIter::get_key_addr() s/b
>>> DCmdArgIter::key_addr(). Similarly, getters such as is_enabled()
>>> should retrieve a field named "_is_enabled" rather than one named
>>> "enabled". You follow the "_is_enabled" convention in other places
>>> such as GenDCmdArgument. Or you could use enabled() to get the
>>> value of the _enabled field.
>>> Also generally, I'd use accessor methods in the implementation of
>>> more complex member methods rather than access the underlying
>>> fields directly. E.g. in GenDCmdArgument::read_value, I'd use
>>> is_set() and set_is_set(true), (set_is_set() is not actually
>>> defined, but should be) rather than directly accessing _is_set.
>>> Though sometimes doing this is too much of a pain with fields whose
>>> type is a template argument, as in the
>>> DCmdArggument<char*>::parse_value() method in
>>> diagnosticArgument.cpp.
>>> For easy readability, it'd be nice to line up field names (the ones
>>> with an _ prefix) at the same column.
>>> On line 200, "instanciated" -> "instantiated"
>>> On line 218, I'd use "heap_allocated" rather than "heap" for the
>>> formal arg name.
>>> On line 248, you could indent the text to start underneath
>>> "outputStream". I generally find that indenting arguments lines
>>> (formal or actual) so they line up with the first argument position
>>> make the code more readable, but I'm not religious about it.
>>> On line 265, "instanciated" -> "instantiated"
>>> DCmdFactorys are members of a singly-linked list, right? If so,
>>> it'd be good to have a comment to that effect on the declaration of
>>> _next.
>>> On line 322, insert a blank before "true". You might fix this in
>>> other places where there's no blank between a comma in an argument
>>> list and the following parameter value.
>>> In diagnosticCommandFramework.cpp:
>>> The code from lines 80-95 and 105-120 is identical. Factor out?
>>> In diagnosticArgument.cpp:
>>> On line 41, insert blanks before the actual arguments. (see above
>>> generic comment)
>>> On line 77, the "if" is indented one space too many.
>>> In management.cpp:
>>> I'd be consistent with having or not having a space between
>>> "while", "if" and "for" and the following "(" in this and your
>>> other code. Most hotspot code has a space.
>>> Thanks,
>>> Paul
>>> On 12/2/11 8:57 AM, Frederic Parain wrote:
>>>> Hi All,
>>>> [Posting to serviceability-dev, runtime-dev and core-libs-dev
>>>> because changes are pretty big and touch all these areas]
>>>> Here's a framework for issuing diagnostics commands to the JVM.
>>>> Diagnostic commands are actions executed inside the JVM mainly
>>>> for monitoring or management purpose. This work has two parts.
>>>> The first part is in the hotspot repository and contains the
>>>> framework itself with two diagnostic commands. The second part is
>>>> in the JDK repository and contains the command line utility to
>>>> invoke diagnostic commands as well as the HotSpotDiagnosticMXBean
>>>> extensions. The HotSpotDiagnosticMXBean extensions allow a remote
>>>> client to discover and invoke diagnostic commands using a JMX
>>>> connection.
>>>> This changeset only contains two diagnostic commands, more
>>>> commands will be added in the future, as a standalone effort to
>>>> improve the monitoring and management services of the JVM, or as
>>>> part of other projects.
>>>> Webrevs are here:
>>>> Here's a technical description of this work:
>>>> 1 - The Diagnostic Command Framework 1-1 Overview
>>>> The diagnostic command framework is fully implemented in native
>>>> code and relies on the HotSpot's internal exception mechanism.
>>>> The rational of a pure native implementation is to be able to
>>>> execute diagnostic commands even in critical situations like an
>>>> OutOfMemory error. All diagnostic commands are registered in a
>>>> single list, and two flags control the way a user can interact
>>>> with them. The "hidden" flag prevents a diagnostic command from
>>>> appearing in the list of available commands returned by the
>>>> "help" command. However, it's still possible to get the detailed
>>>> help message for a hidden command with the "help <command name>"
>>>> syntax (but it requires to know the name of the hidden command).
>>>> The second flag is "enabled" and it controls if a command can be
>>>> invoked or not. When listed with the "help" commands, disabled
>>>> commands appear with a "[disabled]" label in their description.
>>>> If the user tries to invoke a disabled command, an error message
>>>> is returned and the command is not run. This error message can be
>>>> customized on a per command base. The framework just provides
>>>> these two flags with their semantic, it doesn't provide any
>>>> policy or mechanism to set or modify these flags. These actions
>>>> will be delegated to the JVM or special diagnostic commands.
>>>> 1-2 Implementation
>>>> All diagnostic commands are implemented as subclasses of the DCmd
>>>> class defined in services/diagnosticFramework.hpp. Here's the
>>>> layout of the DCmd class and the list of methods that a new
>>>> command has to define or overwrite:
>>>> class DCmd { DCmd(outputStream *output);
>>>> static const char *get_name();
>>>> static const char *get_description();
>>>> static const char *get_disabled_message();
>>>> static const char *get_impact();
>>>> static int get_num_arguments();
>>>> virtual void print_help(outputStream* out);
>>>> virtual void parse(CmdLine* line, char delim, TRAPS);
>>>> virtual void execute(TRAPS);
>>>> virtual void reset(TRAPS);
>>>> virtual void cleanup();
>>>> virtual GrowableArray<const char *>* get_argument_name_array();
>>>> virtual GrowableArray<DCmdArgumentInfo*>*
>>>> get_argument_info_array(); }
>>>> A diagnostic command is always instantiated with an outputStream
>>>> in parameter. This outputStream can point either to a file, a
>>>> buffer or a socket (see the ostream.hpp file).
>>>> The get_name() method returns the string that will identify the
>>>> command (i.e. the string to put on the command line to invoke
>>>> it).
>>>> The get_description() methods returns the global description of
>>>> the command.
>>>> The get_disabled_message() returns the customized message to
>>>> return when the command is disabled, without having to
>>>> instantiate the command.
>>>> The get_impact() returns a description of the intrusiveness of
>>>> the diagnostic command on the Java Virtual Machine behavior. The
>>>> rational for this method is that some diagnostic commands can
>>>> seriously disrupt the behavior of the Java Virtual Machine (for
>>>> instance a Thread Dump for an application with several tens of
>>>> thousands of threads, or a Head Dump with a 40GB+ heap size) and
>>>> other diagnostic commands have no serious impact on the JVM (for
>>>> instance, getting the command line arguments or the JVM version).
>>>> The recommended format for the description is<impact level>:
>>>> [longer description], where the impact level is selected among
>>>> this list: {low, medium, high}. The optional longer description
>>>> can provide more specific details like the fact that Thread Dump
>>>> impact depends on the heap size.
>>>> The get_num_arguments() returns the number of options/arguments
>>>> recognized by the diagnostic command. This method is only used by
>>>> the JMX interface support (see below).
>>>> The print_help() methods prints a detailed help on the
>>>> outputStream passed in argument. The detailed help contains the
>>>> list of all supported options with their type and description.
>>>> The parse() method is in charge of parsing the command arguments.
>>>> Each command is free to implement its own argument parser.
>>>> However, an argument parser framework is provided (see section
>>>> 1-3) to ease the implementation, but its use is optional. The
>>>> parse method takes a delimiter character in argument, which is
>>>> used to mark the limit between two arguments (typically
>>>> invocation from jcmd will use a space character as a delimiter
>>>> while invocation from the JVM command line parsing code will use
>>>> a comma as a delimiter).
>>>> The execute() method is naturally the one to invoke to get the
>>>> diagnostic command executed. The parse() and the execute()
>>>> methods are dissociated, so it's a possible perform the argument
>>>> parsing in one thread, and delegate the execution to another
>>>> thread, as long as the diagnostic command doesn't reference
>>>> thread local variables. The framework allows several instances of
>>>> the same diagnostic command to be executed in parallel. If for
>>>> some reasons concurrent executions should not be allowed for a
>>>> given diagnostic command, this is the responsibility of the
>>>> diagnostic command implementor to enforce this rule, for instance
>>>> by protecting the body of the execute() method with a global
>>>> lock.
>>>> The reset() method is used to initialize the internal fields of
>>>> the diagnostic command or to reset the internal fields to their
>>>> initial value to be able to re-use an already allocated
>>>> diagnostic command instance.
>>>> The cleanup() method is used to perform perform cleanup (like
>>>> freeing of all memory allocated to store internal data). The DCmd
>>>> extends the ResourceObj class, so when allocated in a
>>>> ResourceArea, destructors cannot be used to perform cleanup. To
>>>> ensure that cleanup is performed in all cases, it is recommended
>>>> to create a DCmdMark instance for each DCmd instance. DCmdMark is
>>>> a stack allocated object with a pointer to a DCmd instance. When
>>>> the DCmdMark is destroyed, its destructor calls the cleanup()
>>>> method of the DCmd instance it points to. If the DCmd instance
>>>> has been allocated on the C-Heap, the DCmdMark will also free the
>>>> memory allocated to store the DCmd instance.
>>>> The get_argument_name_array() and get_argument_info_array() are
>>>> related to the JMX interface of the diagnostic command framework,
>>>> so they are described in section 3.
>>>> 1-3 DCmdParser framework
>>>> The DCmdParser class is an optional framework to help development
>>>> of argument parsers. It provides many features required by the
>>>> diagnostic command framework (generation of the help message or
>>>> the argument descriptions for the JMX interface) but all these
>>>> features can easily be re-implemented if a developer decides not
>>>> to use the DCmdParser framework.
>>>> The DCmdParser class is relying on the DCmdArgument template.
>>>> This template must be used to define the different types of
>>>> argument the parser will have to handle. When a new
>>>> specialization of the template is done, three methods have to be
>>>> provided:
>>>> void parse_value(const char *str,size_t len,TRAPS); void
>>>> init_value(TRAPS); void destroy_value();
>>>> The parse_value() method is used to convert a string into an
>>>> argument value. The print_value() method is used to display the
>>>> default value (support for the detailed help message). The
>>>> init_value() method is used to initialize or reset the argument
>>>> value. The destroy_value() method is a clean-up method (useful
>>>> when the argument has allocated some C-Heap memory to store its
>>>> value and this memory has to be freed before destroying the
>>>> DCmdArgument instance).
>>>> The DCmdParser makes a distinction between options and
>>>> arguments. Options are identified by a key name that must appear
>>>> on the command line, while argument are identified just by the
>>>> position of the argument on the command line. Options use
>>>> the<key>=<value> syntax. In case of boolean options, the
>>>> '=<value>' part of the syntax can be omitted to set the option to
>>>> true. Arguments are just sequences characters delimited by a
>>>> separator character. This separator can be specified at runtime
>>>> when invoking the diagnostic command framework. If an argument
>>>> contain a character that could be used as a delimiter, it's
>>>> possible to enclose the argument between single or double quotes.
>>>> Options are arguments are instantiated using the same
>>>> DCmdArgument class but they're registered differently to the
>>>> DCmdParser.
>>>> The way to use the DCmdParser is to declare the parser and the
>>>> option/arguments as fields of the diagnostic command class (which
>>>> is itself a sub-class of the DCmd class), like this:
>>>> class EchoDCmd : public DCmd { protected: DCmdParser
>>>> _dcmdparser;
>>>> DCmdArgument<jlong> _required;
>>>> DCmdArgument<jlong> _intval;
>>>> DCmdArgument<bool> _boolval;
>>>> DCmdArgument<char *> _stringval;
>>>> DCmdArgument<char *> _first_arg;
>>>> DCmdArgument<jlong> _second_arg;
>>>> DCmdArgument<char *> _optional_arg;
>>>> }
>>>> The parser and the options/arguments must be initialized before
>>>> the diagnostic command class, and the options/arguments have to
>>>> be registered to the parser like this:
>>>> EchoDCmd(outputStream *output) : DCmd(output),
>>>> _stringval("-strval","a string argument","STRING",false),
>>>> _boolval("-boolval","a boolean argument","BOOLEAN",false),
>>>> _intval("-intval","an integer argument","INTEGER",false),
>>>> _required("-req","a mandatory integer argument","INTEGER",true),
>>>> _fist_arg("first argument","a string argument","STRING",true),
>>>> _second_arg("second argument,"an integer
>>>> argument,"INTEGER",true),
>>>> _optional_arg("optional argument","an optional string
>>>> argument","STRING","false")
>>>> {
>>>> _dcmdparser.add_dcmd_option(&_stringval)
>>>> _dcmdparser.add_dcmd_option(&_boolval);
>>>> _dcmdparser.add_dcmd_option(&_intval);
>>>> _dcmdparser.add_dcmd_option(&_required);
>>>> _dcmdparser.add_argument(&_first_arg);
>>>> _dcmdparser.add_argument(&_second_arg);
>>>> _dcmdparser.add_argument(&_optional_arg); };
>>>> The add_dcmd_argument()/ add_dcmd_option() method is used to add
>>>> an argument/option to the parser. The option/argument constructor
>>>> takes the name of the option/argument, its description, a string
>>>> describing its type and a boolean to specify if the
>>>> option/argument is mandatory or not. The parser doesn't support
>>>> option/argument duplicates (having the same name) but the code
>>>> currently doesn't check for duplicates.The order used to register
>>>> options has no impact on the parser. However, the order used to
>>>> register arguments is critical because the parser will use the
>>>> same order to parse the command line. In the example above, the
>>>> parser expects to have a first argument of type STRING (parsed
>>>> using _first_arg), then a second argument of type INTEGER (parsed
>>>> using _second_arg) and optionally a third parameter of type
>>>> STRING (parsed using _optional_arg). A mandatory option or
>>>> argument has to be specify every time the command is invoked. If
>>>> it is missing, an exception is thrown at the end of the parsing.
>>>> Optional arguments have to be registered after mandatory
>>>> arguments. An optional argument will be considered for parsing
>>>> only if all arguments before it (mandatory or not) have already
>>>> been used to parse the command line.
>>>> The DCmdParser and its DCmdArgument instances are embedded in the
>>>> DCmd instance. The rational for this design is to limit the
>>>> number of C-heap allocations but also to be able to pre-allocate
>>>> diagnostic command instances for critical situations. If the
>>>> process is running out of C-heap space, it's not possible to
>>>> instantiate new diagnostic commands to troubleshoot the
>>>> situation. By pre-allocating some diagnostic commands, it will be
>>>> possible to run them even in this critical situation. Of course,
>>>> the diagnostic command itself should not try to allocate memory
>>>> during its execution, this prevents the diagnostic command to use
>>>> variable length arguments like strings. By nature, pre-allocated
>>>> diagnostic commands aim to be re-usable, this is the purpose of
>>>> the reset() method which restores the default status of all
>>>> arguments.
>>>> 1-4 Internal invocation
>>>> Using a diagnostic command from the JVM itself is pretty easy:
>>>> instantiate the class and invoke the parse() method then the
>>>> execute() method. A diagnostic command can be instantiated from
>>>> inside the JVM even it is not registered. This is a difference
>>>> with the external invocations (from jcmd or JMX) that require the
>>>> command to be
>>> registered.
>>>> 2 - The JCmd interface
>>>> Diagnostic commands can also be invoked from outside the JVM
>>>> process, using the new 'jcmd' utility. The jcmd program uses the
>>>> attach API to connect to the JVM, send requests and receive
>>>> results. The jcmd utility must be launched on the same machine
>>>> than the one running the JVM (its a local tool). Launched without
>>>> arguments, jcmd displays a list of all JVMs running on the
>>>> machine. The jcmd source code is in the jdk repository like other
>>>> existing j* tools.
>>>> To execute a diagnostic command in a particular JVM, the generic
>>>> syntax is:
>>>> jcmd<pid_of_the_jvm> <command_name> [arguments]
>>>> The attachListener has been modified to recognized the jcmd
>>>> requests. When a jcmd request is identified, it is parsed to
>>>> extract the command name. The JVM performs a look up of this
>>>> command in a list of registered commands. To be executable by an
>>>> external request, a diagnostic command has to be registered. The
>>>> registration is performed with the DCmdFactory class (see
>>>> services/management.cpp).
>>>> 3 - The JMX interface
>>>> The framework provides a JMX based interface to the diagnostic
>>>> commands. This interface allows remote invocation of diagnostic
>>>> commands through a JMX connection.
>>>> 3-1 The interface
>>>> The information related to the diagnostic commands are
>>>> accessible through new methods added to the
>>>> public List<String> getDiagnosticCommands();
>>>> public DiagnosticCommandInfo getDiagnosticCommandInfo(String
>>>> command);
>>>> public List<DiagnosticCommandInfo>
>>>> getDiagnosticCommandInfo(List<String> command);
>>>> public List<DiagnosticCommandInfo> getDiagnosticCommandInfo();
>>>> public String execute(String commandLine) throws
>>>> IllegalArgumentException ;
>>>> public String execute(String cmd, String... arguments) throws
>>>> IllegalArgumentException;
>>>> The getDiagnosticCommands() method returns an array containing
>>>> the names of the not-hidden registered diagnostic commands.
>>>> The three getDiagnosticCommandInfo() methods return one or
>>>> several diagnostic command descriptions using the
>>>> DiagnosticCommandInfo class.
>>>> The two execute() methods allow the user the invoke a diagnostic
>>>> command in different ways.
>>>> The DiagnosticCommandInfo class is describing a diagnostic
>>>> command with the following information:
>>>> public class DiagnosticCommandInfo {
>>>> public String getName();
>>>> public String getDescription();
>>>> public String getImpact();
>>>> public boolean isEnabled();
>>>> public List<DiagnosticCommandArgumentInfo> getArgumentsInfo();
>>>> }
>>>> The getName() method returns the name of the diagnostic command.
>>>> This name is the one to use in execute() methods to invoke the
>>>> diagnostic command.
>>>> The getDescription() method returns a general description of the
>>>> diagnostic command.
>>>> The getImpact() method returns a description of the intrusiveness
>>>> of diagnostic command.
>>>> The isEnabled() method returns true if the method is enabled,
>>>> false if it is disabled. A disabled method cannot be executed.
>>>> The getArgumentsInfo() returns a list of descriptions for the
>>>> options or arguments recognized by the diagnostic command. Each
>>>> option/argument is described by a DiagnosticCommandArgumentInfo
>>>> instance:
>>>> public class DiagnosticCommandArgumentInfo {
>>>> public String getName();
>>>> public String getDescription();
>>>> public String getType();
>>>> public String getDefault();
>>>> public boolean isMandatory();
>>>> public boolean isOption();
>>>> public int getPosition(); }
>>>> If the DiagnosticCommandArgumentInfo instance describes an
>>>> option, isOption() returns true and getPosition() returns -1.
>>>> Otherwise, when the DiagnosticCommandArgumentInfo instance
>>>> describes an argument, isOption() returns false and getPosition()
>>>> returns the expected position for this argument. The position of
>>>> an argument is defined relatively to all arguments passed on the
>>>> command line, options are not considered when defining an
>>>> argument position. The getDefault() method returns the default
>>>> value of the argument if a default has been defined, otherwise it
>>>> returns null.
>>>> 3-2 The implementation
>>>> The framework has been designed in a way that prevents
>>>> diagnostic command developers to worry about the JMX interface.
>>>> In addition to the methods described in section 1-2, a diagnostic
>>>> command developer has to provide three methods:
>>>> int get_num_arguments()
>>>> which returns the number of option and arguments supported by
>>>> the command.
>>>> GrowableArray<const char *>* get_argument_name_array()
>>>> which provides the name of the arguments supported by the
>>>> command.
>>>> GrowableyArray<DCmdArgumentInfo*>* get_argument_info_array()
>>>> which provides the description of each argument with a
>>>> DCmdArgumentInfo instance. DCmdArgumentInfo is a C++ class used
>>>> by the framework to generate the
>>>> instances. This is done
>>>> automatically and the diagnostic command developer doesn't need
>>>> to know how to create Java objects from the runtime.
>>>> 4 - The Diagnostic Commands
>>>> To avoid name collisions between diagnostic commands coming from
>>>> different projects, use of a flat name space should be avoided
>>>> and a more structured organization is recommended. The framework
>>>> itself doesn't depend on this organization, so it will be a set
>>>> of rules defining a convention in the way commands are named.
>>>> Diagnostic commands can easily organized in a hierarchical way,
>>>> so the template for a command name can be:
>>>> <domain>.[sub-domain.]<command>
>>>> This template can be extended with sub-sub-domains and so on.
>>>> A special set of commands without domain will be reserved for
>>>> the commands related to the diagnostic framework itself, like the
>>>> "help" command.
>>>> Thanks,
>>>> Fred

More information about the core-libs-dev mailing list