On Fri, 26 Feb 2021 21:39:48 GMT, Ioi Lam <ik...@openjdk.org> wrote: >> Yumin Qi has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains five additional commits >> since the last revision: >> >> - 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 > > src/hotspot/share/memory/dynamicArchive.cpp line 347: > >> 345: if (Arguments::GetSharedDynamicArchivePath() == NULL) { >> 346: if (!RecordDynamicDumpInfo) { >> 347: // If run with -XX:+RecordDynamicDumpInfo, >> DynamicDumpSharedSpaces will be turned on, > > Is this check needed? It looks like `MetaspaceShared::cmd_dump_dynamic` will > not call `DynamicArchive::dump()` unless the path was set up correctly.
Fixed. The warning is harmless so I just revert it back. > src/hotspot/share/memory/metaspaceShared.cpp line 789: > >> 787: char filename[JVM_MAXPATHLEN]; >> 788: const char* file = file_name; >> 789: assert(strcmp(cmd, "static_dump") == 0 || strcmp(cmd, "dynamic_dump") >> == 0, "Sanity check"); > > Since the caller of this function already performed the string validity > check, I think it's better to pass `bool is_static` as a parameter and not > pass `cmd`. Moved to CDS.java, code is simple than this. > src/hotspot/share/memory/metaspaceShared.cpp line 863: > >> 861: MutexLocker lock(ClassLoaderDataGraph_lock); >> 862: DumpClassListCLDClosure collect_classes(stream); >> 863: ClassLoaderDataGraph::loaded_cld_do(&collect_classes); > > Need to close the stream. Changed to use stack object so it will close the file at destrutor. > src/hotspot/share/runtime/globals.hpp line 1896: > >> 1894: >> \ >> 1895: product(bool, RecordDynamicDumpInfo, false, >> \ >> 1896: "Record class info for jcmd Dynamic dump") >> \ > > "Record class info for jcmd VM.cds dynamic_dump"? Fixed. ------------- PR: https://git.openjdk.java.net/jdk/pull/2737