On Sat, 20 Jul 2024 07:30:55 GMT, Thomas Stuefe <[email protected]> wrote:
>> Sonia Zaldana Calles has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Missing copyright header update
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685308132
