RFR: 8202750: Reduce the use of get_canonical_path() in CDS

[Ioi Lam]

On Tue, 16 Feb 2021 02:04:43 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

> Please review this proposed change which includes:
> 
> - replace calls to `get_canonical_path()` with `ClassLoader::get_native_path()` which calls `os::native_path()`;
> - factor out some code for opening a zip file into `ClassLoader::open_zip_file()`;
> - modify `ClassLoader::get_canonical_path()` so that all buffer allocations are within the function.
> 
> Testing: Tiers 1 - 4.

Changes requested by iklam (Reviewer).

src/hotspot/share/classfile/classLoader.cpp line 1357:

> 1355:       assert(native_path_table_entry != NULL, "sanity");
> 1356:       // If the path (from the class stream source) is the same as the shared
> 1357:       // class or module path, then we have a match.

How about adding the comment:

// src may come from the App/Platform class loaders, which would canonicalize
// the file name. We cannot use strcmp to check for equality against ent->name().
// We must use os::same_files (which is faster than canonicalizing ent->name()).

src/hotspot/share/classfile/classLoader.cpp line 1348:

> 1346:     char* native_path = get_native_path(path);
> 1347:     // The path is from the ClassFileStream. Since a ClassFileStream has been created successfully in functions
> 1348:     // such as ClassLoader::load_class(), its source path must be valid.

This comment was written when the class could only be parsed by the boot loader using ClassFileStream. Now the class can also be parsed by the app/platform class loaders. How about deleting this comment, and change the previous comment block to

// Save the path from the file: protocol or the module name from the jrt: protocol
// if no protocol prefix is found, path is the same as stream->source(). This path
// must be valid since the class has been successfully parsed.

src/hotspot/share/classfile/classLoader.cpp line 1294:

> 1292:   return os::native_path(native_path);
> 1293: }
> 1294: 

If I understand correctly, the reason to call `os::native_path()` is the Windows version of `os::same_files()` cannot handle the difference between "/" (from `src`) and "\" (from `ent->name()`).

I think it's better to move the call to native_path inside the Windows implementation of `os::same_files()`. That way, we don't need to call `os::native_path()` in all the callers of `os::same_files()`.

This would also make it possible to use resource allocation inside `os::same_files()` to allocate the native path (instead of os::strdup).

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

PR: https://git.openjdk.java.net/jdk/pull/2581