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

[Sonia Zaldana Calles]

On Mon, 29 Jul 2024 09:39:07 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 c...
>
> Thanks Thomas @tstuefe -
> 
> We're agreeing that some of this framework is overly complex, and that we aren't going to simplify the framework in this change.
> 
> But the more we adopt the obscure parts of the framework, the the harder it will be to move away from it, so that's the reason for suggesting not creating the FileArgument class.  Use the simpler parts of this machine, with some special cases where necessary, like a char* argument which happens to be used for a FILEname (an input filename which gets %p substitution).
> 
> The logic I don't follow is:
> Using this complex mechanism because it exists, when it only has one? actual usage.  This seems to contradict the earlier max path len notes where it's suggested not to use a pattern established by about 140 other usages.
> 
> Apologies Sonia for dragging this out, still really pleased to get this change happening.

Hi @kevinjwalls, 

I reverted back to `char*` and modified parsing based on the type `FILE`.

Really hope this reaches a consensus!

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

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