On Wed, 31 Mar 2021 20:58:46 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 > > 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
