RFR: 8259070: Add jcmd option to dump CDS

[Calvin Cheung]

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

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