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 Changes requested by iklam (Reviewer). src/hotspot/share/memory/dynamicArchive.cpp line 347: > 345: if (Arguments::GetSharedDynamicArchivePath() == NULL) { > 346: if (!RecordDynamicDumpInfo) { > 347: // If run with -XX:+RecordDynamicDumpInfo, DynamicDumpSharedSpaces > will be turned on, Is this check needed? It looks like `MetaspaceShared::cmd_dump_dynamic` will not call `DynamicArchive::dump()` unless the path was set up correctly. src/hotspot/share/memory/metaspaceShared.cpp line 811: > 809: if (!RecordDynamicDumpInfo) { > 810: output->print_cr("Please run with -Xshare:auto > -XX:+RecordDynamicDumpInfo dumping dynamic archive!"); > 811: return; There are several error conditions: (1) CDS is not configured for this VM build. In this case, `INCLUDE_CDS` is false. The DumpSharedArchiveDCmd should be placed inside `#if INCLUDE_CDS`, so the user won't be able to issue `jcmd VM.cds` at all, and we will never come to here. (2) CDS is configured, but the JVM process is not running with CDS enabled. This could have several causes: - The JVM does not have a built-in archive. - User has specified `-XX:SharedArchiveFile=foo.jsa`, but foo.jsa doesn't exist - The shared archive exists, but has failed to map - `-Xshare:off` is specified in the command-line. I think all of the above can be checked inside metaspce.cpp. Note that if you have specified `DynamicDumpSharedSpaces` but the base archive fails to map, you will get a similar error. if (RecordDynamicDumpInfo && !UseSharedSpaces) { vm_exit_during_initialization("RecordDynamicDumpInfo is unsupported when base CDS archive is not loaded "); } (3) `jcmd VM.cds dynamic_dump` is used on a JVM process without RecordDynamicDumpInfo: We can do the check here, but I think we can change the error message to be more specific: if (!RecordDynamicDumpInfo) { output->print_cr("Unable to dump dynamic shared archive. " "Please restart the JVM with -XX:+RecordDynamicDumpInfo"); } Note that the user can get to (3) with incorrect command-line options such as `java -Xshare:off .....`, but we don't need to list all those conditions here. Instead, if the user follows the suggestion of (3) and add `-XX:+RecordDynamicDumpInfo` to the command-line, they will then get the error message of (2) and will know how to proceed further. src/hotspot/share/memory/metaspaceShared.cpp line 795: > 793: if (file_name ==nullptr) { > 794: os::snprintf(filename, sizeof(filename), "java_pid%d_static.jsa", > os::current_process_id()); > 795: file = filename; I think the above `os::snprintf` can be combined for both dynamic and static case: int n = os::snprintf(filename, sizeof(filename), "java_pid%d_%s.jsa", os::current_process_id() (is_static ? "static" : "dynamic"); assert(n < sizeof(filename), "should not truncate"); `snprintf` man page says "a return value of size or more means that the output was truncated." src/hotspot/share/memory/metaspaceShared.cpp line 799: > 797: if (strstr(file_name, ".jsa") == nullptr) { > 798: os::snprintf(filename, sizeof(filename), "%s.jsa", file_name); > 799: file = filename; This could potentially overflow the buffer. I think it's best to just leave `file_name` alone. If the user doesn't want the `.jsa` extension, that's fine. Similarly, we don't add `.jsa` to `-XX:ArchiveClassesAtExit` or `-XX:SharedArchiveFile`. 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); Need to check for truncation. We should also add a test case for a very long file names specified in "jcmd VM.cds ...." src/hotspot/share/memory/metaspaceShared.cpp line 868: > 866: return; > 867: } > 868: os::snprintf(exec_path, sizeof(exec_path), Need to check for buffer overflow. I think it's better to use a stringStream so you don't need to worry about buffer allocation and overflow. Also, if any of the arguments you append to the command line contain space characters, `os::fork_and_exec` is not going to work. I think it's best to check for space characters (and maybe other special characters such as single quote, double quote, $, etc) and return an error (something like `"special character "%c" in the command-line is not supported"`) src/hotspot/share/memory/metaspaceShared.cpp line 881: > 879: } > 880: char* buff_start = exec_path + strlen(exec_path); > 881: snprintf(buff_start, sizeof(exec_path), " -cp %s %s", app_class_path, > java_command); Do we need to pass `java_command`? src/hotspot/share/memory/metaspaceShared.cpp line 889: > 887: if (DynamicArchive::has_been_dumped_once()) { > 888: output->print_cr("Dynamic dump has been done, and should only be > done once."); > 889: return; How about "Dynamic dump cannot be done more than once"? src/hotspot/share/memory/metaspaceShared.cpp line 898: > 896: ArchiveClassesAtExit = file; > 897: if (Arguments::init_shared_archive_paths()) { > 898: DynamicArchive::dump(); Instead of modifying the global `ArchiveClassesAtExit`, I think it's better to change `DynamicArchive::dump()` to take an argument of the file to write. `SharedDynamicArchivePath` should be changed to be used only when mapping the dynamic archive (not for writing). src/hotspot/share/oops/instanceKlass.cpp line 4249: > 4247: if (is_hidden() || unsafe_anonymous_host() != NULL) { > 4248: return false; > 4249: } Maybe `InstanceKlass::log_to_classlist` should be refactored to use this function? Also, I think a better name would be `bool InstanceKlass::can_be_logged_to_classlist()`. src/hotspot/share/runtime/arguments.cpp line 3138: > 3136: FLAG_SET_DEFAULT(DynamicDumpSharedSpaces, false); > 3137: } else { > 3138: FLAG_SET_DEFAULT(DynamicDumpSharedSpaces, true); I think this will be more readable: if (ArchiveClassesAtExit != NULL || RecordDynamicDumpInfo) { FLAG_SET_DEFAULT(DynamicDumpSharedSpaces, true); } else { FLAG_SET_DEFAULT(DynamicDumpSharedSpaces, false); BTW, what happens if you specify `-XX:-DynamicDumpSharedSpaces -XX:+RecordDynamicDumpInfo`? src/hotspot/share/runtime/arguments.cpp line 3525: > 3523: os::free(SharedDynamicArchivePath); > 3524: SharedDynamicArchivePath = nullptr; > 3525: } Is this necessary? src/hotspot/share/runtime/globals.hpp line 1896: > 1894: > \ > 1895: product(bool, RecordDynamicDumpInfo, false, > \ > 1896: "Record class info for jcmd Dynamic dump") > \ "Record class info for jcmd VM.cds dynamic_dump"? src/hotspot/share/services/diagnosticCommand.cpp line 1084: > 1082: > 1083: DumpSharedArchiveDCmd::DumpSharedArchiveDCmd(outputStream* output, bool > heap) : > 1084: DCmdWithParser(output, heap), Should be inside `#if INCLUE_CDS` src/hotspot/share/memory/metaspaceShared.cpp line 789: > 787: char filename[JVM_MAXPATHLEN]; > 788: const char* file = file_name; > 789: assert(strcmp(cmd, "static_dump") == 0 || strcmp(cmd, "dynamic_dump") > == 0, "Sanity check"); Since the caller of this function already performed the string validity check, I think it's better to pass `bool is_static` as a parameter and not pass `cmd`. src/hotspot/share/memory/metaspaceShared.cpp line 863: > 861: MutexLocker lock(ClassLoaderDataGraph_lock); > 862: DumpClassListCLDClosure collect_classes(stream); > 863: ClassLoaderDataGraph::loaded_cld_do(&collect_classes); Need to close the stream. ------------- PR: https://git.openjdk.java.net/jdk/pull/2737
