On Fri, 26 Feb 2021 00:03:40 GMT, Yumin Qi <mi...@openjdk.org> wrote:
> Hi, Please review > > Added jcmd option for dumping CDS archive during application runtime. > Before this change, user has to dump shared archive in two steps: first run > application with > `java -XX:DumpLoadedClassList=<classlist> .... ` > to collect shareable class names and saved in file `<classlist>` , then > `java -Xshare:dump -XX:SharedClassListFile=<classlist> > -XX:SharedArchiveFile=<archivefile> ...` > With this change, user can use jcmd to dump CDS without going through above > steps. Also user can choose a moment during the app runtime to dump an > archive. > The bug is associated with the CSR: > https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved. > New added jcmd option: > `jcmd <pid or AppName> VM.cds static_dump <filename>` > or > `jcmd <pid or AppName> VM.cds dynamic_dump <filename>` > To dump dynamic archive, requires start app with newly added flag > `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to > dynamic dump like loader constraints will be recorded. Note the dumping > process changed some object memory locations so for dumping dynamic archive, > can only done once for a running app. For static dump, user can dump multiple > times against same process. > The file name is optional, if the file name is not supplied, the file name > will take format of `java_pid<number>_static.jsa` or > `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The > `<number>` is the application process ID. > > Tests: tier1,tier2,tier3,tier4 > > Thanks > Yumin Looks like a good usability enhancement to CDS. Some comments below... Thanks, Calvin Below are my comments... src/hotspot/share/memory/metaspaceShared.cpp line 783: > 781: char* start = buffer + strlen(buffer); > 782: snprintf(start, buff_len, "%s ", arg); > 783: } Maybe move the above function to the StringUtils class under share/utilities? Use `os::snprintf()` instead of `snprintf()`? src/hotspot/share/memory/metaspaceShared.cpp line 788: > 786: // The existing file will be overwritten. > 787: char filename[JVM_MAXPATHLEN]; > 788: const char* file = file_name; Is the variable at line 788 necessary? Could you just pass filename to callees? src/hotspot/share/memory/metaspaceShared.cpp line 801: > 799: file = filename; > 800: } > 801: } This block of code is very similar to lines 813 - 821 below. Maybe factor it into another function? src/hotspot/share/memory/metaspaceShared.cpp line 831: > 829: DumpClassListCLDClosure(fileStream* f) : CLDClosure() { _stream = f; } > 830: ~DumpClassListCLDClosure() { > 831: delete _stream; // The file need close since in child process it > will be used. Can you clarify the above comment? src/hotspot/share/memory/metaspaceShared.cpp line 856: > 854: char classlist_name[JVM_MAXPATHLEN]; > 855: > 856: os::snprintf(classlist_name, sizeof(classlist_name), "%s.classlist", > file); I think the `file` contains the ".jsa" suffix. So the `classlist_name` would be <name>.jsa.classlist. Maybe only the prefix of `file` should be passed into cmd_dump_static() and cmd_dump_dynamic() and have the functions append the suffix for the names for the classlist and archive? test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTest.java line 213: > 211: if (!cdsEnabled) { > 212: System.out.println("CDS is not available for this JDK, skip > the test."); > 213: return; Should throw SkippedException instead. ------------- Changes requested by ccheung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2737
