On Sat, 20 Jul 2024 07:30:55 GMT, Thomas Stuefe <stu...@openjdk.org> 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