RFR: 8328313: Archived module graph should allow identical --module-path to be specified during dump time and run time [v4]

[Ioi Lam]

On Tue, 24 Sep 2024 21:20:16 GMT, Calvin Cheung <ccheung at 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