On Tue, 24 Sep 2024 21:20:16 GMT, Calvin Cheung <cche...@openjdk.org> wrote:
>> Prior to this patch, if `--module-path` is specified in the command line: >> during CDS dump time, full module graph will not be included in the CDS >> archive; >> during run time, full module graph will not be used. >> >> With this patch, the full module graph will be included in the CDS archive >> with the `--module-path` option. During run time, if the same >> `--module-path` option is specified, the archived module graph will be used. >> >> The checking of module paths between dump time and run time is more lenient >> compared with the checking of class paths; the ordering of the modules is >> unimportant, duplicate module names are ignored. >> E.g. the following is considered a match: >> dump time runtime >> m1,m2 m2,m1 >> m1,m2 m1,m2,m2 >> >> I included some >> [notes](https://bugs.openjdk.org/browse/JDK-8328313?focusedId=14699275&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14699275) >> in the bug report regarding some changes in the corelib classes. > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > fix indentation src/hotspot/share/cds/filemap.cpp line 956: > 954: } > 955: // module paths are stored in sorted order in the CDS archive. > 956: module_paths->sort(ClassLoaderExt::compare_module_path_by_name); I think it's better to put this call inside `ClassLoaderExt::extract_jar_files_from_path` src/hotspot/share/cds/heapShared.cpp line 879: > 877: > 878: ResourceMark rm(THREAD); > 879: if ((strcmp(k->name()->as_C_string(), > "jdk/internal/module/ArchivedModuleGraph") == 0) && You can avoid the ResourceMark by if (k->name()->equals("jdk/internal/module/ArchivedModuleGraph") src/hotspot/share/cds/heapShared.cpp line 885: > 883: log_info(cds, heap)("Skip initializing ArchivedModuleGraph subgraph: > is_using_optimized_module_handling=%s num_module_paths=%d", > 884: > BOOL_TO_STR(CDSConfig::is_using_optimized_module_handling()), > ClassLoaderExt::num_module_paths()); > 885: return; I think we can add a comment like: ArchivedModuleGraph was created with a --module-path that's different than the runtime --module-path. Thus, it might contain references to modules that do not exist in runtime. We cannot use it. src/hotspot/share/classfile/classLoader.cpp line 582: > 580: false /*is_boot_append */, false > /* from_class_path_attr */); > 581: if (new_entry != nullptr) { > 582: assert(new_entry->is_jar_file(), "module path entry %s is not a jar > file", new_entry->name()); How do we guarantee that new_entry is never a JAR file? Do we never come here if --module-path points to an exploded directory? A comment would be helpful. src/hotspot/share/classfile/classLoaderExt.cpp line 152: > 150: DIR* dirp = os::opendir(path); > 151: if (dirp == nullptr && errno == ENOTDIR && has_jar_suffix(path)) { > 152: module_paths->append(path); Does this handle the case where `path` doesn't exist? src/hotspot/share/classfile/classLoaderExt.cpp line 162: > 160: int n = os::snprintf(full_name, full_name_len, "%s%s%s", path, > os::file_separator(), file_name); > 161: assert((size_t)n == full_name_len - 1, "Unexpected number of > characters in string"); > 162: module_paths->append(full_name); Can this case be handled: --module-path=dir - Dump time : dir contains only mod1.jar - Run time : dir contains only mod1.jar and mod2.jmod src/hotspot/share/runtime/arguments.cpp line 347: > 345: } > 346: } > 347: return false; Can this be simplified to `return (strcmp(key, MODULE_PROPERTY_PREFIX PATH) == 0)`? src/java.base/share/classes/jdk/internal/loader/BuiltinClassLoader.java line 1092: > 1090: void resetArchivedStatesForAppClassLoader() { > 1091: setClassPath(null); > 1092: if (!moduleToReader.isEmpty()) moduleToReader.clear(); Suggestion: if (!moduleToReader.isEmpty()) { moduleToReader.clear(); } Also, do we need to do the same thing for the platform loader as well? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1774242056 PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1774243535 PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1774245579 PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1774247339 PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1774248778 PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1774249819 PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1774251713 PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1774252333
