On Mon, 22 Jul 2024 20:03:08 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: > > Error messaging format Slowly getting there... :) src/hotspot/share/prims/wbtestmethods/parserTests.cpp line 79: > 77: DCmdArgument<char*>* argument = new DCmdArgument<char*>( > 78: name, desc, > 79: "STRING", mandatory, default_value); I would revert all these style-only changes and just keep the functional one (addition of FileArgument handling). Let's keep this for a follow-up. src/hotspot/share/services/diagnosticArgument.cpp line 376: > 374: THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), > error_msg.base()); > 375: } > 376: } The realloc here is a bit pointless since if `_value._name` is set, it already points to a buffer of size JVM_MAXPATHLEN. I would either one of these two: - either inline the buffer into FileArgument as I wrote earlier; no need to allocate or deallocate then. - or, in this function, allocate if _name is not null, use existing buffer otherwise src/hotspot/share/services/diagnosticCommand.cpp line 524: > 522: HeapDumper dumper(!_all.value() /* request GC if _all is false*/); > 523: dumper.dump(_filename.value()._name, output(), (int)level, > _overwrite.value(), > 524: (uint)parallel); Please revert style-only changes, lets keep those for follow ups. src/hotspot/share/services/diagnosticCommand.cpp line 1195: > 1193: > 1194: void SystemDumpMapDCmd::execute(DCmdSource source, TRAPS) { > 1195: const char* name = _filename.value()._name; This direct access to the member inside _filename is a bit awkward. I would make the buffer private and give the class some getters and setters, possibly like this: class FileArgument { // private stuff public: const char* get() const { // return internal buffer } // returns true if parsing succeeded, false if not bool parse_value(const char* s, size_t len) { // call Arguments::copyexpand, target internal buffer, and return its return value } } ------------- PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2193132206 PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1687527542 PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1688310305 PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1688264948 PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1688316839
