On Tue, 13 Apr 2021 23:04:24 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 16 commits: > > - Fix too much verbose output, make call to stopApp after test > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - Restructed code and rename LingeredTestApp.java to JCmdTestLingeredApp.java > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - 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 > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/008fc75a...88c0f7d1 Looks good. Just have a few minor comments. test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTest.java line 86: > 84: bootJar = JarBuilder.build("boot", BOOT_CLASSES); > 85: Path testJarPath = FileSystems.getDefault().getPath(testJar); > 86: Path bootJarPath = FileSystems.getDefault().getPath(bootJar); I think the above could be replaced with: String testJarPath = TestCommon.getTestJar(“test.jar”); String bootJarPath = TestCommon.getTestJar(“boot.jar”); Most of the CDS tests use the `getTestJar` method. You can leave it as is if you like. test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTest.java line 149: > 147: args.add("-Xlog:class+load"); > 148: > 149: LingeredApp app = createLingeredApp(args.toArray(new String[0])); Instead of `new String[0]`, I think it is more intuitive to use `new String[args.size()]`. ------------- Marked as reviewed by ccheung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2737
