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

Chris Plummer chris.plummer at oracle.com
Fri Jun 15 05:04:13 UTC 2018


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.

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





More information about the hotspot-runtime-dev mailing list