RFR: 8259070: Add jcmd option to dump CDS [v8]

[Ioi Lam]

On Wed, 31 Mar 2021 20:58:46 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
>
> Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
> 
>  - Fix revert unintentionally comment, merge master.
>  - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
>  - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. Removed unused function from ClassLoader. Improved InstanceKlass::is_shareable() and related test. Added more test scenarios.
>  - Remove redundant check for if a class is shareable
>  - Fix according to review comment and add more tests
>  - Fix filter more flags to exclude in static dump, add more test cases
>  - Merge branch 'master' into jdk-8259070
>  - Fix white space in CDS.java
>  - Add function CDS.dumpSharedArchive in java to dump shared archive
>  - 8259070: Add jcmd option to dump CDS

Changes requested by iklam (Reviewer).

src/hotspot/share/classfile/vmSymbols.hpp line 304:

> 302:   template(generateLambdaFormHolderClasses_signature, "([Ljava/lang/String;)[Ljava/lang/Object;") \
> 303:   template(dumpSharedArchive, "dumpSharedArchive")                                                \
> 304:   template(dumpSharedArchive_signature, "(ZLjava/lang/String;)V")                                 \

Need to align the "dumpSharedArchive" part with the previous line.

src/hotspot/share/prims/jvm.cpp line 3745:

> 3743: #if INCLUDE_CDS
> 3744:   assert(UseSharedSpaces && RecordDynamicDumpInfo, "already checked in arguments.cpp?");
> 3745:   if (DynamicArchive::has_been_dumped_once()) {

Maybe add a comment like this:? 
// During dynamic archive dumping, some of the data structures are overwritten so
// we cannot dump the dynamic archive again. TODO: this should be fixed.

src/hotspot/share/prims/jvm.cpp line 3754:

> 3752:   assert(ArchiveClassesAtExit == nullptr, "already checked in arguments.cpp?");
> 3753:   Handle file_handle(THREAD, JNIHandles::resolve_non_null(archiveName));
> 3754:   char* archive_name  = java_lang_String::as_utf8_string(file_handle());

A ResourceMark is needed before calling java_lang_String::as_utf8_string().

In general, I think the code in jvm.cpp should only marshall the jobject argument (e.g., convert `jstring` to `char*`.). The main functionality of JVM_DumpDynamicArchive should be moved to dynamicArchive.cpp. Similarly, most of the work in JVM_DumpClassListToFile should be moved to metaspaceShared.cpp.

src/hotspot/share/prims/jvm.cpp line 3759:

> 3757:     DynamicArchive::dump();
> 3758:   } else {
> 3759:     THROW_MSG(vmSymbols::java_lang_RuntimeException(),

Need to set ArchiveClassesAtExit to NULL before throwing the exception, since dynamic dump may not work anymore after the failure.

test/hotspot/jtreg/runtime/cds/appcds/jcmd/LingeredTestApp.java line 28:

> 26: public class LingeredTestApp extends LingeredApp {
> 27:     // Do not use default test.class.path in class path.
> 28:     public boolean useDefaultClasspath() { return false; }

It's not obvious that you're changing the behavior of the base class by overriding a member function. It's better to have 

public LingeredTestApp() {
   setUseDefaultClasspath(false);
}

Also, the name of LingeredTestApp is kind of generic. How about renaming it to JCmdTestLingeredApp?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737