[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8259070: Add jcmd option to dump CDS [v8]
From:       Ioi Lam <iklam () openjdk ! java ! net>
Date:       2021-03-31 23:30:42
Message-ID: 3yTFtGkXVbbpeMoqSZjS8Gu_iYUmDVFzUjqX6hCI0Ro=.942e390b-672f-4090-bf0a-59b937b23e60 () github ! com
[Download RAW message or body]

On Wed, 31 Mar 2021 20:58:46 GMT, Yumin Qi <minqi@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


[prev in list] [next in list] [prev in thread] [next in thread]