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

[Calvin Cheung]

On Wed, 25 Sep 2024 00:11:32 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> 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`

My thinking was since the function may return false at line 953, the entries in the `module_paths` doesn't need to be sorted until before calling `check_paths()`. Anyway, I've made the change you suggested.

> 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")

Done.

> 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.

Added the comment.

> 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.

I've added the following comment:
  `// ClassLoaderExt::process_module_table() filters out non-jar entries before calling this function.`

> 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?

If the `path` doesn't exist, `dirp` will be nullptr and it will go to the else case. I think `os::readir` on `nullptr` should return a `nullptr`. To make the code clearer, I've added a `nullptr` check on `dirp` in the else case.

> 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

It should work because the jmod file won't be added to the `module_paths`.

> 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)`?

I'm not sure. Is your suggest equivalent to:
  `return (strcmp(key, "jdk.module.path"));`

> 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?

Added braces.
The `setClassPath(null)` used to be in `ClassLoaders.AppClassLoader`. Based on investigations so far, the clearing of the `moduleToReader` map is required only for `AppClassLoader`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1776147825
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1776147896
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1776147991
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1776148021
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1776148168
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1776148232
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1776148308
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1776148461