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

[Frederic Parain]

Hi Dan,

Thank you for the review.

The new webrev is here:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.05/

And my answers are in-lined below.

On 12/12/11 09:08 PM, Daniel D. Daugherty wrote:
> 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...

Fixed

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

Fixed

> 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).

Fixed

> 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?

Threads have two areas to store handles, one for internal
handles and one for JNI handles. The HandleMarkCleaner
applies only to the internal handles. The purpose of
the make_local() is to create a JNI handle from the
internal handle, so when the internal area is cleaned
up by the HandleMarkCleaner, the JNI handle is still
valid in the context of the caller.

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

Fixed

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

Fixed

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

Fixed

> 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... :-))

I've removed all default parameters I found.

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

Fixed

> lines 31-33 - should be some indent here

Fixed

> line 74: It would be useful to display the command that
> can't be found:
>
> help foo
> Help unavailable: 'foo': No such command

Done.

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

I was not aware of the performance penalty. However, this method
is called only once in the lifetime of the VM, so I didn't fixed it.

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

Fixed

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

Fixed.

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

Yes, I got several reviews about the style :-)

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

Removed

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

OK. Fixed

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

The method doesn't really return a local variable, this is a
return by value. The return statement invokes the copy constructor
to duplicate the local object into the caller context. Here, the
default copy constructor is called but it's okay because the CmdLine
class has be designed to work this way. I've added a comment in the
code to make this mechanism more explicit.

> line 155: missing a space after '='

Fixed

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

Fixed

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

Fixed

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

Fixed

> 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.

You're right, this is definitively odd.
I've fixed _cmd_len and updated line 58.

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

Fixed

> line 318 - what is 'idx' used for?

Remaining code from a previous version.
Unused now, so I removed it.

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

Fixed


Thank you,

Fred

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