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

Thomas Stüfe thomas.stuefe at gmail.com
Sun Jun 17 05:48:08 UTC 2018 

Thank you Coleen. I pushed.

The document reads a bit outdated in places:

<quote>Be sparing with templates.</quote>

I think that ship has sailed :P

Best Regards, Thomas



On Sat, Jun 16, 2018 at 12:07 AM,  <coleen.phillimore at oracle.com> wrote:
>
>
> On 6/15/18 4:52 PM, Thomas Stüfe wrote:
>>
>> On Fri, Jun 15, 2018 at 10:36 PM, Chris Plummer
>> <chris.plummer at oracle.com> wrote:
>>>
>>> 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?
>>>
>> For me, it is a safety valve against missing initialization. Consider
>> this:
>>
>> class Parser { const char* p; .. };
>>
>> Parser(some input) {
>>    // complicated stuff happening, p gets initialized in all paths. Ok.
>> }
>>
>> evolves to this:
>>
>> Parser(some input) {
>>    // more complicated stuff, on one path p may not get initialized.
>> }
>>
>> if p does not get initialized, its value is essentially random, at
>> least in relase builds. Especially if Parser is placed on the stack,
>> since p then is mapped to whatever happened to be at this stack slot
>> before.
>>
>> So, by always initializing it to NULL:
>>
>> Parser(some input) : p(NULL) {
>>    // complicated stuff happening, p gets initialized in all paths
>> }
>>
>> from the beginning, I get reproducable errors. Using p == NULL will
>> crash, using p == random-and-maybe-readable-pointer may do random
>> things or even work out of accident.
>>
>> Thanks, Thomas
>>
>
> I agree with Thomas and I think the change looks good.  Thank you for
> humoring me with the punctuation.  It doesn't seem to be here.
>
> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
>
> Thanks,
> Coleen
>
>
>>
>>
>>
>>
>>
>>
>>> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>

More information about the serviceability-dev mailing list