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

[Sonia Zaldana Calles]

On Sat, 20 Jul 2024 08:02:34 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> src/hotspot/share/services/diagnosticArgument.hpp line 66:
>> 
>>> 64:   public:
>>> 65:     char *_name;
>>> 66: };
>> 
>> Something is off about this. What is the lifetime of this object?
>> 
>> You don't free it. Running a command in a loop will consume C-heap (you can check this with NMT: `jcmd VM.native_memory baseline`, then run a command 100 times, then `jcmd VM.native_memory summary.diff` will show you the leak in mtInternal.
>> 
>> I would probably just inline the string. E.g.
>> 
>> 
>> struct FileArgument {
>>   char name[max name len]
>> };
>> 
>> 
>> FileArguments sits as member inside DCmdArgument. DCmdArgument or DCmdArgumentWithParser sits as member in the various XXXDCmd classes. 
>> 
>> Those are created in DCmdFactory::create_local_DCmd(). Which is what, a static global list? So we only have one global XXXDCmd object instance per command, but for each command invocation re-parse the argument values? What a weird concept.
>> 
>> Man, this coding is way too convoluted for a little parser engine :( 
>> 
>> But anyway, inlining the filename array into FileArgument should be probably fine from a size standpoint. I would, however, not use JVM_MAXPATHLEN or anything that depends ultimately on PATH_MAX from system headers. We don't want the object to consume e.g. an MB if some crazy platform defines PATH_MAX as 1MB. Therefore I would use e.g. 1024 as limit for the path name.
>> 
>> (Note that PATH_MAX is an illusion anyway, there is never a guarantee that a path is smaller than that limit... See this good article: https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html)
>
> Note that the reason for the leak is probably the fact that you don't clear old values on parse_value. See e.g. how char* does it. However, since you allocate with a constant size anyway, the buffer size never changes, you could just as well either follow my advice above (inlining), or just re-use the existing pointer.

Hi Thomas, 

Yes - this was an oversight on my end. I was not directly calling the `destroy_value()` function. I tried to follow more closely what’s done for `char*`, as I like the consistency throughout. 

I did a quick check and I don’t see any more leaks with NMT. Does the new change make sense to you as well? 

Thank you for the feedback!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1687081245