RFR(s): 8204958: Minor cleanups for the diagnostic framework

[Thomas Stüfe]

Hi all,

(I moved the issue to target release 12 - if we get this through the
review quick, fine, but no reason to add pressure)

some more background: The reason for these cleanups is that I later
plan to add a new sub command which allows us to run multiple commands
in one safepoint (see [1] for more details and discussions). Since
this will require tweaking the parse process a bit, I try to loosen up
the coding before adding complexity again.

Please find here the new webrev:

Delta:  http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00-to-01/webrev/
Full:   http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.01/webrev/

What changed:

Aesthetics:
- I reordered member init lists to member order, as Coleen did ask. I
also reform them as lists, just to increase readability.
- For a number of inline methods, I aligned their implementations so
that they too would be better readable and it would be clearer which
methods are const
- Moved DCmdFactory::_DCmdFactoryList up to the other static members.

Functional:
- Added initializer list for CmdLine
- Added missing initializations for DCmdArgIter members _key_addr,
_key_len, _value_addr, _value_len, as Coleen suggested
- constified DCmdInfo too - since this is an immutable value holder,
all members can be const.
- also in DCmdInfo, changed JavaPermission permission() to const
JavaPermission& permission() to save a bit unnecessary copying.
- Fixed initialization of DCmdArgumentInfo::_position as Serguei and
Chris requested
- Removed default implementation of DCmd::name and DCmd::description.
There is no good reason to provide one - all child classes should (and
do) provide an implementation. I rather have linkage errors if someone
forgets to add one.
- Removed default implementations for DCmdWithParser::name/description
- same reasoning as above
- Removed DCmdWithParser::permission(), it is already provided by the
base class.
- Removed DCmdWithParser::num_arguments() since childs of
DCmdWithParser will always have arguments and hence should implement
this function.
- Made all functions in DCmdFactoryImpl non-virtual. There is no point
in allowing to override them.

--

Notes:

I see the following commands do not implement "permission" and hence
rely on the default impl of DCmd::permission, which returns a
permission structure { NULL, NULL, NULL } - is that okay and intended?

JMXStopRemoteDCmd
JMXStartLocalDCmd
JMXStartRemoteDCmd
TouchedMethodsDCmd
ClassStatsDCmd
RunFinalizationDCmd
SystemGCDCmd
VMUptimeDCmd
HelpDCmd

I am not sure where and how these permissions are tested.

Thinking through this further:

Of the 47 child classes of DCmd, 37 seem to return
"java.lang.management.ManagementPermission", name "monitor". So, we
have this function 37 times:

  static const JavaPermission permission() {
    JavaPermission p = {"java.lang.management.ManagementPermission",
                        "monitor", NULL};
    return p;
  }

Would it then not make sense to make this the default implementation
for DCmd::permission() - instead of the NULL permission - and only
require child DCmds to provide a permission if it differs from this
default? We could then loose all 37 methods.

---

Similar thoughts for "static DCmd impact()"

which is not provided by:

JMXStatusDCmd
JMXStopRemoteDCmd
JMXStartLocalDCmd

which thus return the default impact - DCmd::impact() - which returns "Low".

Here I think we should either remove the default implementation and
force all commands to provide an implementation or stay with the
default "Low" implementation and remove all 20+ functions in child
classes which also return "Low".

--

Finally, some thoughts for possible future cleanups:

GenDCmdArgument internally uses C-heap to hold copies of strings
(DCmdArgument<char*> and DCmdArgument<StringArrayArgument>). I wonder
whether we could use resource area instead. Then we could get rid of
all cleanup code (GenDCmdArgument::cleanup(),
GenDCmdArgument::destroy_value()) etc. And if one agrees that parsing
is only done once and the commands are not reused (which is the case
currently) we also may get rid of DCmdArgument::reset() and friends.
Coding would become a lot easier.

--

IMHO the way object- und template based polymorphy is mixed is a bit
of a mismatch here and it complicates things a bit.

For instance, Dcmd has above mentioned static functions which return
immutable attributes for the commands. It is "overwritten"
template-style by redefining the same static functions in derived
classes and called via DCmdFactoryImpl<command class>.

Maybe it is just me, but I find this a bit roundabout for the problem
it tries to solve. Also, it is really difficult to parse for modern
IDEs - e.g. my CDT is unable to tell me who calls "DCmd::permission()"
since the relation between DCmd::permission() and XXXCmd::permission()
is just by name. Also, why pay for the text size of 47 just slightly
different template definitions of DCmdFactory<xxx> where one class
would be enough?

The mis-fit between templates and object polymorphism also shows up as
weird artifacts. E.g. we have 27 variants of this function:

int XXXXDCmd::num_arguments() {
  ResourceMark rm;
  XXXDCmd* dcmd = new SetVMFlagDCmd(NULL, false);
  if (dcmd != NULL) {
    DCmdMark mark(dcmd);
    return dcmd->_dcmdparser.num_arguments();
  } else {
    return 0;
  }
}

- so, we want to implement "static XXXDCmd::num_arguments", which -
being static - has to create an object of the same type to ask it its
number of arguments.

Maybe we could un-templify the command classes at some point to
simplify all this?

I think we need two things, a class to hold meta information about the
command - name, description, argument types etc - which needs to be
available at startup. And then, we need a second thing to hold runtime
context: the concrete argument values the user gave us, the
outputStream* pointer, the execute() function, maybe parser state...

I think this may be expressed simpler and more efficiently with simple
C++ objects and inheritance, without templates. We could also shrink
the code quite a bit. And my CDT would find who calls "permissions()"
:)


Best Regards, Thomas

-------

[1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023673.html


On Wed, Jun 13, 2018 at 3:41 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> Hi all,
>
> while working on jcmd I saw some minor cleanup possibilities.
>
> Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8204958
>
> Most of them are const correctness and using initializer lists instead
> of java-style member initialization in these C++ classes.
>
> Other changes:
>
> - Added a ResourceMark into the DCmd::parse_and_execute() loop
> - removed DCmdFactory::create_global_DCmd() (and following that,
> DCmdFactory::create_Cheap_instance()) since I did not find anyone
> using that function.
> - Also in DCmdFactory, I removed a number of setters which were never
> called and for attributes which should be immutable: hidden, flags,
> enabled.
>
> This is really a rather light cleanup. It did not touch any larger issues.
>
> Thank you,
>
> Thomas