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

Fri Jun 15 20:36:19 UTC 2018

On 6/15/18 10:31 AM, Thomas Stüfe wrote:
> 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.
I tend to think the opposite. Using the initializer list in this way 
would lead me to think that the field was not initialized in the 
constructor body, or at the very least there was a path where it might 
not be initialized. I don't see how this helps when the constructor is 
not obvious. Is the point of the initializer (in this case) solely to 
have the code document which fields get initialized in the constructor?

thanks,

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