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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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

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

Commit messages:
 - Merge branch 'master' into 8202750-dir-canonical-form
 - replace get_canonical_path() with get_native_path() in ClassLoader::record_result()
 - fix the DirClasspathTest.java test failure on Windows
 - simply the fix
 - 8202750 (initial commit)

Changes: https://git.openjdk.java.net/jdk/pull/2581/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2581&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8202750
  Stats: 60 lines in 2 files changed: 21 ins; 18 del; 21 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2581.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2581/head:pull/2581

PR: https://git.openjdk.java.net/jdk/pull/2581
Reply | Threaded
Open this post in threaded view
|

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

Yumin Qi-3
On Tue, 16 Feb 2021 02:04:43 GMT, Calvin Cheung <[hidden email]> 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.

LGTM.

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

Marked as reviewed by minqi (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2581
Reply | Threaded
Open this post in threaded view
|

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

Ioi Lam-2
In reply to this post by Calvin Cheung
On Tue, 16 Feb 2021 02:04:43 GMT, Calvin Cheung <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8202750: Reduce the use of get_canonical_path() in CDS [v2]

Calvin Cheung
In reply to this post by Calvin Cheung
> 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.

Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:

  per review comments from @iklam

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2581/files
  - new: https://git.openjdk.java.net/jdk/pull/2581/files/dc755372..bf940008

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2581&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2581&range=00-01

  Stats: 38 lines in 3 files changed: 17 ins; 14 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2581.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2581/head:pull/2581

PR: https://git.openjdk.java.net/jdk/pull/2581
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8202750: Reduce the use of get_canonical_path() in CDS [v2]

Calvin Cheung
In reply to this post by Ioi Lam-2
On Tue, 16 Feb 2021 16:51:11 GMT, Ioi Lam <[hidden email]> wrote:

>> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   per review comments from @iklam
>
> 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()).

Fixed.

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

Fixed.

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

I've added `os::native_path()` calls in the Windows version of `os::same_files()`.
`os::strip()` and `os::free()` are still being used because ResourceMark may not be available during early VM startup.

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

PR: https://git.openjdk.java.net/jdk/pull/2581
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8202750: Reduce the use of get_canonical_path() in CDS [v2]

Ioi Lam-2
In reply to this post by Calvin Cheung
On Wed, 17 Feb 2021 16:45:54 GMT, Calvin Cheung <[hidden email]> 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.
>
> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
>
>   per review comments from @iklam

Marked as reviewed by iklam (Reviewer).

src/hotspot/os/windows/os_windows.cpp line 4488:

> 4486:   native_file1 = os::native_path(native_file1);
> 4487:   char* native_file2 = os::strdup(file2);
> 4488:   native_file2 = os::native_path(native_file2);

`os::strdup` should `be os::strdup_check_oom`

Other than that, this PR looks good.

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

PR: https://git.openjdk.java.net/jdk/pull/2581
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8202750: Reduce the use of get_canonical_path() in CDS [v2]

Calvin Cheung
On Wed, 17 Feb 2021 22:18:45 GMT, Ioi Lam <[hidden email]> wrote:

>> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   per review comments from @iklam
>
> src/hotspot/os/windows/os_windows.cpp line 4488:
>
>> 4486:   native_file1 = os::native_path(native_file1);
>> 4487:   char* native_file2 = os::strdup(file2);
>> 4488:   native_file2 = os::native_path(native_file2);
>
> `os::strdup` should `be os::strdup_check_oom`
>
> Other than that, this PR looks good.

Fixed. I'll do some testing before integrate.

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

PR: https://git.openjdk.java.net/jdk/pull/2581
Reply | Threaded
Open this post in threaded view
|

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

Calvin Cheung
In reply to this post by Calvin Cheung
On Tue, 16 Feb 2021 02:04:43 GMT, Calvin Cheung <[hidden email]> 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.

This pull request has now been integrated.

Changeset: 5f308291
Author:    Calvin Cheung <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/5f308291
Stats:     72 lines in 3 files changed: 27 ins; 21 del; 24 mod

8202750: Reduce the use of get_canonical_path() in CDS

Reviewed-by: minqi, iklam

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

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