RFR: 8259070: Add jcmd option to dump CDS

Ioi Lam iklam at openjdk.java.net
Fri Feb 26 23:06:42 UTC 2021 

On Fri, 26 Feb 2021 00:03:40 GMT, Yumin Qi <minqi at 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

More information about the hotspot-runtime-dev mailing list