On Wed, 25 Sep 2024 00:11:32 GMT, Ioi Lam <ik...@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
