Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8202750: Reduce the use of get_canonical_path() in CDS #2581

Closed

Conversation

calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Feb 16, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8202750: Reduce the use of get_canonical_path() in CDS

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2581/head:pull/2581
$ git checkout pull/2581

@calvinccheung
Copy link
Member Author

calvinccheung commented Feb 16, 2021

/label remove hotspot
/label add hotspot-runtime

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 16, 2021

👋 Welcome back ccheung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Feb 16, 2021

@calvinccheung The hotspot label was not set.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 16, 2021
@openjdk
Copy link

openjdk bot commented Feb 16, 2021

@calvinccheung
The hotspot-runtime label was successfully added.

@calvinccheung calvinccheung marked this pull request as ready for review Feb 16, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 16, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 16, 2021

Webrevs

yminqi
yminqi approved these changes Feb 16, 2021
Copy link
Contributor

@yminqi yminqi left a comment

LGTM.

@openjdk
Copy link

openjdk bot commented Feb 16, 2021

@calvinccheung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8202750: Reduce the use of get_canonical_path() in CDS

Reviewed-by: minqi, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 107 new commits pushed to the master branch:

  • ea5bf45: 8261621: Delegate Unicode history from JLS to j.l.Character
  • d5a4d22: 8261843: incorrect info in docs/building.html
  • bf75a3a: 8261851: update ReflectionCallerCacheTest.java test to use ForceGC from test library
  • 05301f5: 8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280
  • cb84539: 8261553: Efficient mask generation using BMI2 BZHI instruction
  • a065879: 8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
  • 9ba2b71: 8261657: [PPC64] Cleanup StoreCM nodes after CMS removal
  • f639df4: 8261401: Add sanity check for UseSHM large pages similar to the one used with hugetlb large pages
  • 2e18b52: 8261752: Multiple GC test are missing memory requirements
  • c7885eb: 8261758: [TESTBUG] gc/g1/TestGCLogMessages.java fails if ergonomics detect too small InitialHeapSize
  • ... and 97 more: https://git.openjdk.java.net/jdk/compare/becee6435bc38c4e3fe5b197c985e68e97fc8e0d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 16, 2021
// A shared path has been validated during its creation in ClassLoader::create_class_path_entry(),
// it must be valid here.
assert(success, "must be valid path");
assert(native_path_table_entry != NULL, "sanity");
// If the path (from the class stream source) is the same as the shared
// class or module path, then we have a match.
Copy link
Member

@iklam iklam Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@calvinccheung calvinccheung Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

// 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()
char* path = skip_uri_protocol(src);
char* canonical_class_src_path = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, char, JVM_MAXPATHLEN);
bool success = get_canonical_path(path, canonical_class_src_path, JVM_MAXPATHLEN);
char* native_path = get_native_path(path);
// The path is from the ClassFileStream. Since a ClassFileStream has been created successfully in functions
// such as ClassLoader::load_class(), its source path must be valid.
Copy link
Member

@iklam iklam Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@calvinccheung calvinccheung Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

assert(native_path != NULL, "sanity");
return os::native_path(native_path);
}

Copy link
Member

@iklam iklam Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@calvinccheung calvinccheung Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

iklam
iklam approved these changes Feb 17, 2021
char* native_file1 = os::strdup(file1);
native_file1 = os::native_path(native_file1);
char* native_file2 = os::strdup(file2);
native_file2 = os::native_path(native_file2);
Copy link
Member

@iklam iklam Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os::strdup should be os::strdup_check_oom

Other than that, this PR looks good.

Copy link
Member Author

@calvinccheung calvinccheung Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I'll do some testing before integrate.

@calvinccheung
Copy link
Member Author

calvinccheung commented Feb 18, 2021

/integrate

@openjdk openjdk bot closed this Feb 18, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 18, 2021
@openjdk
Copy link

openjdk bot commented Feb 18, 2021

@calvinccheung Since your change was applied there have been 107 commits pushed to the master branch:

  • ea5bf45: 8261621: Delegate Unicode history from JLS to j.l.Character
  • d5a4d22: 8261843: incorrect info in docs/building.html
  • bf75a3a: 8261851: update ReflectionCallerCacheTest.java test to use ForceGC from test library
  • 05301f5: 8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280
  • cb84539: 8261553: Efficient mask generation using BMI2 BZHI instruction
  • a065879: 8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
  • 9ba2b71: 8261657: [PPC64] Cleanup StoreCM nodes after CMS removal
  • f639df4: 8261401: Add sanity check for UseSHM large pages similar to the one used with hugetlb large pages
  • 2e18b52: 8261752: Multiple GC test are missing memory requirements
  • c7885eb: 8261758: [TESTBUG] gc/g1/TestGCLogMessages.java fails if ergonomics detect too small InitialHeapSize
  • ... and 97 more: https://git.openjdk.java.net/jdk/compare/becee6435bc38c4e3fe5b197c985e68e97fc8e0d...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5f30829.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
3 participants