On Sat, 20 Jul 2024 08:02:34 GMT, Thomas Stuefe <stu...@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
