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

Fri Jun 15 17:31:53 UTC 2018

Coleen, Chris,

thanks for the review. Please find the latest webrev:

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

I changed the copyright dates, the format of the initializer lists (I
looked around and used what the most common denominator seemed to be -
still, Coleen, should you ever dig up that style guide of yours, that
would be valuable), and initialized CmdLine::_cmd to line as Coleen
sugggested.

@Chris
> I'm not certain why you added default field initializers for CmdLine::CmdLine when each field is explicitly initialized within the method body.

See Coleens response. I usually prefer it that way if the constructor
is not extremely obvious, to prevent missing initializations should
the constructor body evolve. C++ compiler should optimize that away
anyway.

Thanks for the reviews,

Best Regards, Thomas

On Fri, Jun 15, 2018 at 1:53 PM,  <coleen.phillimore at oracle.com> wrote:
>
>
> On 6/15/18 1:04 AM, Chris Plummer wrote:
>
> Hi Thomas,
>
> I'm not certain why you added default field initializers for
> CmdLine::CmdLine when each field is explicitly initialized within the method
> body.
>
>
> I think the C++ optimizer would elide any unnecessary initializations and it
> seems safe in case new fields are added, although
>
>   38   : _cmd(NULL)
>
>
> could be initialized as _cmd(line).
>
> Can I bikeshed and say I don't like the style where the , are at the
> beginning of the lines?   We don't use this style in other places, that I've
> noticed, and I really don't like reading the punctuation at the beginning of
> the line.   If I could find our open source wiki for the coding style among
> the broken openjdk wiki pages, I would see if the punctuation rule is there.
> One initializer per line is ok.
>
> Thanks,
> Coleen
>
>
> A number of copyrights need updating.
>
> The rest of the changes look good.
>
> thanks,
>
> Chris
>
> On 6/14/18 1:38 AM, Thomas Stüfe wrote:
>
> 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
>
>
>
>
>