On Fri, 19 Jul 2024 14:07:12 GMT, Sonia Zaldana Calles <szald...@openjdk.org> wrote:
>> Hi all, >> >> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492) >> enabling jcmd diagnostic commands that issue an output file to accept the >> `%p` pattern in the file name and substitute it for the PID. >> >> This PR addresses the following diagnostic commands: >> - [x] Compiler.perfmap >> - [x] GC.heap_dump >> - [x] System.dump_map >> - [x] Thread.dump_to_file >> - [x] VM.cds >> >> Note that some jcmd diagnostic commands already enable this functionality >> (`JFR.configure, JFR.dump, JFR.start and JFR.stop`). >> >> I propose opening a separate issue to track updating the man page similarly >> to how it’s done for the JFR diagnostic commands. For example, >> >> >> filename (Optional) Name of the file to which the flight recording >> data is >> written when the recording is stopped. If no filename is >> given, a >> filename is generated from the PID and the current date >> and is >> placed in the directory where the process was started. The >> filename may also be a directory in which case, the >> filename is >> generated from the PID and the current date in the >> specified >> directory. (STRING, no default value) >> >> Note: If a filename is given, '%p' in the filename will be >> replaced by the PID, and '%t' will be replaced by the >> time in >> 'yyyy_MM_dd_HH_mm_ss' format. >> >> >> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265), >> sources for the jcmd manpage remain in Oracle internal repos so this PR >> can’t address that. >> >> Testing: >> >> - [x] Added test case passes. >> - [x] Modified existing VM.cds tests to also check for `%p` filenames. >> >> Looking forward to your comments and addressing any diagnostic commands I >> might have missed (if any). >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Missing copyright header update Moistly good too. Remarks inline. src/hotspot/share/services/diagnosticArgument.cpp line 384: > 382: _value._name = nullptr; > 383: } > 384: } Whatever this `DCmdArgument<FileArgument>::destroy_value()` is supposed to do, it clearly isn't working, since we leak the memory. 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) src/hotspot/share/services/diagnosticArgument.hpp line 113: > 111: void to_string(MemorySizeArgument f, char* buf, size_t len) const; > 112: void to_string(StringArrayArgument* s, char* buf, size_t len) const; > 113: void to_string(FileArgument f, char *buf, size_t len) const; Here, and in all other places: Please use 'char* var', not 'char *var'. ------------- PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2189782275 PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685301655 PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685297940 PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685290041
