RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v14]

[Thomas Stuefe]

On Fri, 26 Jul 2024 11:39:02 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

> One more thing that's troubling me. (Apologies it's now and not last week.)
> 
> I was looking at the _filename.value().get() usage and finding it uncomfortable, compared to the previous simple _filename.value() 8-) Harder to remember and to read and understand. Maybe we can avoid the two accessors, it really is just a char*.
> 
> These additional argument types look like part of the framework which never found an audience: MemorySizeArgument has one usage in CompilationMemoryStatisticDCmd, NanoTimeArgument looks unused -- so the two-accessor usage is only in once place until now?
> 
> Adding FileArgument as another of these might be the wrong direction, as these classes are so almost redundant.
> 
> What if we didn't add FileArgument, and kept using <char*> for _filename args/opts:
> 
> Then in DCmdArgument<char*>::parse_value(), recognise a "FILE" argument type and call Arguments::copy_expand_pid there, to set _value.
> 
> Just seeing if we can cut down some of the complexity here, as Thomas mentioned, it is already very complex for what it is!
> 
> (There is also the to_string method which seemed like it would be useful here, but it needs a buffer so is more complex than calling two accessors... Another thing that seems to part of the framework that was never much adopted.)

IMHO for a functional addition we should follow the established pattern. Reworking the framework is certainly useful, but I would like it if we could get this done first (I intend to use it in other DCmds). 

And if we simplify this coding, we should think first about how to do this and what to solve. Things that come to mind:

- overuse of template
- The argument-type-by-template-division and the runtime "type" string argument (the third argument to DCmdArgument) seem redundant
- the fact that we keep command metadata (which should be constant) together with command invocation data (values for arguments that are scoped to a single command invocation) in a single global structure, and then rewrite the latter every time we invoke a command. That is a strange concept and makes cleaning up temporary memory non-trivial
- the fact that each new command takes a ton of boilerplate coding (Just see the many many repetitions in diagnosticCommand.cpp)
- the fact that we use runtime-polymorphy, which is completely fine, but then all metadata information are "static". So in order to e.g. know how many arguments a command takes, you need to know the command class, since you cannot just call e.g. `num_arguments()` on a `DCmdWithParser*`. I think the whole framework could be done without templates, just using plain old virtual functions instead. This is not code where a vtable lookup really hurts.

Just my random thoughts. Maybe there is more, but my point is that if we agree this can be improved, it would be better in a separate RFE, and not mixed into functional RFEs. 

@lmesnik 
> Also I would recommend to get approval from svc team reviewer.

Who could this be, @plummercj ?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/20198#issuecomment-2253809524