On 12/12/11 8:56 AM, Frederic Parain wrote:
Minor updates:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.04/

src/share/vm/services/attachListener.cpp
    The new include should be in alpha-order (between lines 36 & 37).
    I'm pretty sure that's the new style...

    Block comment on lines 152-155 should be in '//' style and not
    in JavaDoc style (/** ... */)


src/share/vm/services/jmm.h
    lines 192-208 - HotSpot convention is to mimic the existing style
        in a file. For these structs, the field names should all be
        lined up (see 181-188 for an example).

src/share/vm/services/management.cpp
    line 2126 calls JNIHandles::make_local(), but this is a JVM_ENTRY
        wrapped function which means it depends on VM_ENTRY_BASE which
        has a HandleMarkCleaner in it. Since this is a make_local()
        call, won't the local JNIHandle get scrubbed on the way back
        to the caller?

    line 2231 also calls JNIHandles::make_local() from a JVM_ENTRY
        wrapped function. Same local JNIHandle scrubbing issue?

src/share/vm/services/diagnosticArgument.hpp
    lines 45-50 - too much indent; should be 4 spaces

src/share/vm/services/diagnosticArgument.cpp
    HotSpot style is generally 'if (' and not 'if('

src/share/vm/services/diagnosticCommand.hpp
    lines 28-36 - includes should be in alpha order

    lines 44, 64 - Parameter defaults in new code is generally frowned
        upon in HotSpot. They're used when adding a parameter to existing
        code to avoid changing existing calls where the default is OK.
        (I happen to disagree with that last part and I think that all
        calls should be updated just to make sure your reviewers see
        all uses, but I'm just one cranky voice... :-))

src/share/vm/services/diagnosticCommand.cpp
    line 28: includes should be in alpha order

    lines 31-33 - should be some indent here

    line 74: It would be useful to display the command that
        can't be found:

            help foo
            Help unavailable: 'foo': No such command

        or something like that.

    line 101: just FYI, a ResourceMark without a Thread parameter can
        be expensive. Not an issue for HelpDCmd::num_arguments().

    line 103: HotSpot style is generally 'if (' and not 'if('

src/share/vm/services/diagnosticFramework.hpp
    line 38: typo: 'provides method' -> 'provides methods'
    line 40: typo: 'keywor' -> 'keyword'

    lines 43-46 - fields nicely indented here, but not in other new
        header files. Any particular reason?

    lines 48, 154, 218, 286, 324 - Parameter defaults again.

    line 55: is_stop should be:
            return !is_empty() && strncmp("stop", _cmd, _cmd_len) == 0;
        !strncmp() is frowned upon and spaces between params

    line 82 - returns a local variable; that shouldn't work.

    line 155: missing a space after '='

    lines 256, 258: HotSpot style is generally 'if (' and not 'if('

    line 304: typo: 'DCmdFActory' -> 'DCmdFactory'

line 306: typo: 'command to be executed' -> 'command from being executed'

src/share/vm/services/diagnosticFramework.cpp
    line 55: _cmd_len = cmd_end - line;
        This length includes any leading white space that was skipped
        on lines 42-44. It seems odd that '_cmd' points to the first
        non-whitespace in the command, but _cmd_len includes the
        leading white space. If you change the _cmd_len calculation,
        then you have to also change the logic on line 58 so that
        args_len is also not too long.

    lines 79, 104: typo: 'simple' -> 'single'

    line 318 - what is 'idx' used for?

    line 367: HotSpot style is generally 'if (' and not 'if('
    lines 371, 372, 380, 403, 416: space after comma

Dan



Fred

On 12/12/11 03:29 PM, Frederic Parain wrote:
Hi Paul,

Thank you for the review.
I've applied all your recommendations except the refactoring
in diagnosticCommandFramework.cpp (too few lines can be really
factored out without passing many arguments).

New webrev is here:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.03/

Regards,

Fred

On 12/ 8/11 07:26 PM, Paul Hohensee wrote:
For the hotspot part at
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.00/

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:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.00/
http://cr.openjdk.java.net/~fparain/7104647/webrev.jdk.00/

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
com.sun.management.HotspotDiagnosticMXBean:

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 sun.com.management.DcmdArgumentInfo 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



Reply via email to