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

Fri Jun 15 11:53:15 UTC 2018

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180615/5ff178ef/attachment-0001.html>