Thanks Fred.

I don't think maintaining the style of existing code needs to extend to all aspects of that code but is more about general layout, spacing and naming. So adding unnecessary casts to malloc, or declaring all variables at the start of the method are not things that need to be repeated forever in my opinion.

But I won't push for further changes.

Cheers,
David

On 14/12/2011 12:32 AM, Frederic Parain wrote:
Hi David,

Thanks for the review.

Changes have been applied to this new webrev:
http://cr.openjdk.java.net/~fparain/7104647/webrev.jdk.05/

My answers are in-lined below.

Fred

On 12/13/11 07:49 AM, David Holmes wrote:
Hi Fred,

A couple of minor comments on the JDK side:

HotSpotDiagnosticMXBean.java:

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?

List are more convenient to use.
The VM returns a jobjectArray to avoid the creation of a new dependency
between the VM code and the java.util.List class.

HotSpotDiagnostic.java:

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.

Right. The null check has been removed.

---

jmm.h:

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

Fixed

---

hotspotDiagnostic.c:

39 Java_sun_management_HotSpotDiagnostic_getDiagnosticCommands0
40 (JNIEnv *env, jobject dummy)

Put on one line.

But then it goes over the 80 columns limit. I used a formatting
consistent with the dumpHeap method above.

42 if((jmm_version > JMM_VERSION_1_2_1)

Space after 'if'

Fixed

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

Align arg with opening parentheses

Fixed

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.

I agree. But I've tried to be consistent with existing code in the
directory, like GcInfoBuilder.c

61 dcmd_arg_info_array = (dcmdArgInfo*) malloc(num_arg *
sizeof(dcmdArgInfo));

Cast is not needed.

All calls to malloc in this directory have a cast.

---

General: I suggest generating the javadoc for your new classes and
having an editorial check done. I spotted a couple of typos eg:

DiagnosticCommandArgumentInfo.java:
32 * of one parameter of diagnostic command.
^the
DiagnosticCommandInfo.java
89 * Returns the a list of the
^^^^ typo

These two typos have been fixed. I'm re-re-re-reading the javadoc
looking for more :-)

Thanks,

Fred

On 13/12/2011 12:31 AM, Frederic Parain wrote:
Hi Robert,

These issues should be solved with the new webrevs:

http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.03/
http://cr.openjdk.java.net/~fparain/7104647/webrev.jdk.03/

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-...@openjdk.java.net;
core-libs-dev@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net Subject: Re: Request for
Review (XXL): 7104647: Adding a diagnostic command framework

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