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

8261941: Use ClassLoader for unregistered classes during -Xshare:dump #5458

Closed

Conversation

calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Sep 10, 2021

Before this change, unregistered classes are loaded by the boot class loader during CDS dump time.
This RFE creates an URLClassLoader based on the source specified in the classlist and uses the URLClassLoader to load the unregistered class during CDS dump time. The URLClassLoader instances will be cached in a hash table with the source as the key so that classes associated with the same source will be loaded by the same instance of class loader.

Passed tiers 1 - 4 testing.


Progress

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

Issue

  • JDK-8261941: Use ClassLoader for unregistered classes during -Xshare:dump

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5458/head:pull/5458
$ git checkout pull/5458

Update a local copy of the PR:
$ git checkout pull/5458
$ git pull https://git.openjdk.java.net/jdk pull/5458/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5458

View PR using the GUI difftool:
$ git pr show -t 5458

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5458.diff

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Sep 10, 2021

/label remove hotspot
/label add hotspot-runtime

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 10, 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 openjdk bot commented Sep 10, 2021

@calvinccheung The hotspot label was not set.

@openjdk openjdk bot added the hotspot-runtime label Sep 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 10, 2021

@calvinccheung
The hotspot-runtime label was successfully added.

@calvinccheung calvinccheung marked this pull request as ready for review Sep 10, 2021
@openjdk openjdk bot added the rfr label Sep 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 10, 2021

Webrevs

@@ -466,16 +473,16 @@ InstanceKlass* ClassListParser::load_class_from_source(Symbol* class_name, TRAPS
_interfaces->length(), k->local_interfaces()->length());
}

// This tells JVM_FindLoadedClass to not find this class.
Copy link
Member

@iklam iklam Sep 10, 2021

Choose a reason for hiding this comment

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

This comment can be deleted.

The class k used to be defined by the boot loader. As a result, when JVM_FindLoadedClass looked up a class in the boot/platform/app loaders, it may inadvertently find k.

However, after this PR, k is no longer defined by the boot loader.

Copy link
Member Author

@calvinccheung calvinccheung Sep 14, 2021

Choose a reason for hiding this comment

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

Removed the comment.

return k;
}

class URLClassLoaderTable : public ResourceHashtable<
Copy link
Member

@iklam iklam Sep 10, 2021

Choose a reason for hiding this comment

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

I think we should try to decouple the loading of unregistered classes from ClassLoader/ClassloaderExt. Maybe these new functions can be placed in a new file, shared/cds/unregisteredClasses.cpp, and the API entry point would be UnregisteredClasses::load_class(name, path).

Also, ClassLoaderExt::find_classpath_entry_from_cache shouldn't be needed anymore. If the JAR file doesn't exist, URLClassLoader will throw an exception, so we don't need to create a ClassPathEntry just to check for the JAR file's existence.

I wonder if there's any code in classLoader.cpp that was used only for supporting the loading of unregistered classes. If so, it can be removed, too.

Copy link
Member Author

@calvinccheung calvinccheung Sep 14, 2021

Choose a reason for hiding this comment

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

I think we should try to decouple the loading of unregistered classes from ClassLoader/ClassloaderExt. Maybe these new functions can be placed in a new file, shared/cds/unregisteredClasses.cpp, and the API entry point would be UnregisteredClasses::load_class(name, path).

I've moved the new code to unregistedClasses.[c|h]pp.

Also, ClassLoaderExt::find_classpath_entry_from_cache shouldn't be needed anymore. If the JAR file doesn't exist, URLClassLoader will throw an exception, so we don't need to create a ClassPathEntry just to check for the JAR file's existence.

As we've discussed off-line, after removing the ClassLoaderExt::find_classpath_entry_from_cache, the calculation of the length and crc of the ClassFileSteam will be performed in jvm_define_class_common().

I wonder if there's any code in classLoader.cpp that was used only for supporting the loading of unregistered classes. If so, it can be removed, too.

I don't see any functions in classLoader.cpp could be removed. However, I think the ClassLoader::record_result() could be simplified since it is no longer called with unregistered classes. I'd prefer handle the simplification in a follow-up bug.


class URLClassLoaderTable : public ResourceHashtable<
Symbol*, Handle,
7, // prime number
Copy link
Member

@iklam iklam Sep 10, 2021

Choose a reason for hiding this comment

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

I think 7 is too small. Maybe 137?

Copy link
Member Author

@calvinccheung calvinccheung Sep 14, 2021

Choose a reason for hiding this comment

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

Ok. I've increased it to 137.


Handle ClassLoaderExt::create_and_add_url_classloader(Symbol* path, TRAPS) {
Handle url_classloader = create_url_classloader(path, CHECK_NH);
bool added = _url_classloader_table->put(path, url_classloader);
Copy link
Member

@iklam iklam Sep 10, 2021

Choose a reason for hiding this comment

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

added is not used. Did you want to assert that added == true?

I think it's better to move the body of this function to line 342. Then it would be clear that the entry doesn't exist (and there's no need to check for added).

Copy link
Member Author

@calvinccheung calvinccheung Sep 14, 2021

Choose a reason for hiding this comment

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

Modified per your suggestions and I don't need to keep the bool variable.

if (_url_classloader_table == NULL) {
_url_classloader_table = new (ResourceObj::C_HEAP, mtClass)URLClassLoaderTable();
Handle url_classloader = create_and_add_url_classloader(path, CHECK_NH);
return url_classloader;
Copy link
Member

@iklam iklam Sep 10, 2021

Choose a reason for hiding this comment

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

I think the above 2 lines should be removed and the creation should be done in a single place. This means _url_classloader_table->get() would be unnecessarily called when the table is created, but I think that's OK since this happens only once.

Copy link
Member Author

@calvinccheung calvinccheung Sep 14, 2021

Choose a reason for hiding this comment

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

Done.

CHECK_NULL);
assert(result.get_type() == T_OBJECT, "just checking");
oop obj = result.get_oop();
InstanceKlass* k = InstanceKlass::cast(java_lang_Class::as_Klass(obj));
Copy link
Member

@iklam iklam Sep 10, 2021

Choose a reason for hiding this comment

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

No need for the temp variable k since it's immediately returned.

Copy link
Member Author

@calvinccheung calvinccheung Sep 14, 2021

Choose a reason for hiding this comment

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

Fixed.

…_class_misc_info() is being called in ClassLoader::record_result();

2. remove UnregisteredClasses::seen_classloader();
3. remove call to ClassLoaderExt::record_result() in ClassListParser::load_class_from_source() since it is being called in ClassLoader::record_result().
iklam
iklam approved these changes Sep 15, 2021
Copy link
Member

@iklam iklam left a comment

Looks good overall. Just two minor comments.

if (k->local_interfaces()->length() != _interfaces->length()) {
print_specified_interfaces();
print_actual_interfaces(k);
error("The number of interfaces (%d) specified in class list does not match the class file (%d)",
_interfaces->length(), k->local_interfaces()->length());
}

k->clear_shared_class_loader_type();
Copy link
Member

@iklam iklam Sep 15, 2021

Choose a reason for hiding this comment

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

I think this may not be necessary, as the correct loader type should have been set inside ClassLoaderExt::record_result(). Maybe change this to?

assert(k->is_shared_unregistered_class(), "must be");

Copy link
Member Author

@calvinccheung calvinccheung Sep 15, 2021

Choose a reason for hiding this comment

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

I've changed it to assert. Also removed the clear_shared_class_loader_type() since it is not used anymore.
The class loader type is not set not due to the call to ClassLoaderExt::record_result() which will default the loader type to boot loader. For unregistered classes, the ClassLoaderExt::record_result() won't be called in ClassLoader::record_result().

} else {
return false;
}
return add_unregistered_class(current, k);
Copy link
Member

@iklam iklam Sep 15, 2021

Choose a reason for hiding this comment

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

Since this function is now just a pass-through to add_unregistered_class(), maybe we should delete this function, and make add_unregistered_class() public?

Copy link
Member Author

@calvinccheung calvinccheung Sep 15, 2021

Choose a reason for hiding this comment

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

I've made the change. The add_unregistered_class() is already declared public.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 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:

8261941: Use ClassLoader for unregistered classes during -Xshare:dump

Reviewed-by: iklam, minqi

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 114 new commits pushed to the master branch:

  • 7e92abe: 8273710: Remove redundant stream() call before forEach in jdk.jdeps
  • 59b2478: 8273659: Replay compilation crashes with SIGSEGV since 8271911
  • 5e4d09c: 8273300: Check Mutex ranking during a safepoint
  • c86e24d: 8271954: C2: assert(false) failed: Bad graph detected in build_loop_late
  • 14dc517: 8273372: Remove scavenge trace message in psPromotionManager
  • 241ac89: 8273606: Zero: SPARC64 build fails with si_band type mismatch
  • 181292d: 8273801: Handle VMTYPE for "core" VM variant
  • 09ecb11: 8273806: compiler/cpuflags/TestSSE4Disabled.java should test for CPU feature explicitly
  • 99cfc16: 8273805: gc/g1/TestGCLogMessages.java test should handle non-JFR configs
  • 1c5de8b: 8273807: Zero: Drop incorrect test block from compiler/startup/NumCompilerThreadsCheck.java
  • ... and 104 more: https://git.openjdk.java.net/jdk/compare/7e662e7b9d7ea5113f568418e0acac4234ebfb88...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 label Sep 15, 2021
iklam
iklam approved these changes Sep 15, 2021
yminqi
yminqi approved these changes Sep 15, 2021
Copy link
Contributor

@yminqi yminqi left a comment

LGTM.

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Sep 16, 2021

@iklam, @yminqi Thanks for the review.
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2021

Going to push as commit 12fa707.
Since your change was applied there have been 114 commits pushed to the master branch:

  • 7e92abe: 8273710: Remove redundant stream() call before forEach in jdk.jdeps
  • 59b2478: 8273659: Replay compilation crashes with SIGSEGV since 8271911
  • 5e4d09c: 8273300: Check Mutex ranking during a safepoint
  • c86e24d: 8271954: C2: assert(false) failed: Bad graph detected in build_loop_late
  • 14dc517: 8273372: Remove scavenge trace message in psPromotionManager
  • 241ac89: 8273606: Zero: SPARC64 build fails with si_band type mismatch
  • 181292d: 8273801: Handle VMTYPE for "core" VM variant
  • 09ecb11: 8273806: compiler/cpuflags/TestSSE4Disabled.java should test for CPU feature explicitly
  • 99cfc16: 8273805: gc/g1/TestGCLogMessages.java test should handle non-JFR configs
  • 1c5de8b: 8273807: Zero: Drop incorrect test block from compiler/startup/NumCompilerThreadsCheck.java
  • ... and 104 more: https://git.openjdk.java.net/jdk/compare/7e662e7b9d7ea5113f568418e0acac4234ebfb88...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 16, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2021

@calvinccheung Pushed as commit 12fa707.

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

@calvinccheung calvinccheung deleted the 8261941-cds-custom-loader branch Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
3 participants