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.stu...@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
