RFR(s): 8204958: Minor cleanups for the diagnostic framework
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Jun 15 22:07:17 UTC 2018
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 hotspot-runtime-dev
mailing list