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

8262046: Clean up parallel class loading code and comments #3200

Closed
wants to merge 6 commits into from
@@ -322,30 +322,30 @@ Klass* SystemDictionary::resolve_array_class_or_null(Symbol* class_name,
return k;
}

static inline void log_circularity_error(PlaceholderEntry* probe) {
static inline void log_circularity_error(Thread* thread, PlaceholderEntry* probe) {
LogTarget(Debug, class, load, placeholders) lt;
if (lt.is_enabled()) {
ResourceMark rm;
ResourceMark rm(thread);
LogStream ls(lt);
This conversation was marked as resolved by coleenp

This comment has been minimized.

@lfoltan

lfoltan Apr 1, 2021
Member

Is it helpful to pass THREAD in for the ResourceMark?

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021
Author

Yes, I have a THREAD handy.

ls.print("ClassCircularityError detected for placeholder ");
probe->print_entry(&ls);
ls.cr();
}
}

// Must be called for any super-class or super-interface resolution
// Must be called for any superclass or superinterface resolution
// during class definition to allow class circularity checking
// super-interface callers:
// superinterface callers:
// parse_interfaces - from defineClass
// super-class callers:
// superclass callers:
// ClassFileParser - from defineClass
This conversation was marked as resolved by coleenp

This comment has been minimized.

@lfoltan

lfoltan Apr 1, 2021
Member

picky review comment for line #340, comments below do not use a hyphenated "super-class", but instead one work "superclass".

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021
Author

Ok. How about a space? Because the sentence has super-class and super-interface, so I want to change them both to super class and super interface. Or should it be superclass and superinterface ?

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021
Author

Lois pointed out that the spec uses superclass and superinterface, so I changed the comments to use this convention.

// load_shared_class - while loading a class from shared archive
// resolve_instance_class_or_null:
// via: handle_parallel_super_load
// when resolving a class that has an existing placeholder with
// a saved superclass [i.e. a defineClass is currently in progress]
// If another thread is trying to resolve the class, it must do
// super-class checks on its own thread to catch class circularity and
// superclass checks on its own thread to catch class circularity and
// to avoid deadlock.
//
// resolve_super_or_fail adds a LOAD_SUPER placeholder to the placeholder table before calling
@@ -360,8 +360,8 @@ InstanceKlass* SystemDictionary::resolve_super_or_fail(Symbol* class_name,
bool is_superclass,
TRAPS) {

assert(super_name != NULL, "null super class for resolving");
assert(!Signature::is_array(super_name), "invalid super class name");
assert(super_name != NULL, "null superclass for resolving");
assert(!Signature::is_array(super_name), "invalid superclass name");
#if INCLUDE_CDS
if (DumpSharedSpaces) {
// Special processing for handling UNREGISTERED shared classes.
@@ -373,7 +373,7 @@ InstanceKlass* SystemDictionary::resolve_super_or_fail(Symbol* class_name,
}
#endif // INCLUDE_CDS

// If klass is already loaded, just return the super class or interface.
// If klass is already loaded, just return the superclass or superinterface.
// Make sure there's a placeholder for the class_name before resolving.
// This is used as a claim that this thread is currently loading superclass/classloader
// and for ClassCircularity checks.
@@ -398,10 +398,10 @@ InstanceKlass* SystemDictionary::resolve_super_or_fail(Symbol* class_name,
(quicksuperk->class_loader() == class_loader()))) {
return quicksuperk;
} else {
// Must check ClassCircularity before checking if super class is already loaded.
// Must check ClassCircularity before checking if superclass is already loaded.
PlaceholderEntry* probe = placeholders()->get_entry(name_hash, class_name, loader_data);
if (probe && probe->check_seen_thread(THREAD, PlaceholderTable::LOAD_SUPER)) {
log_circularity_error(probe);
log_circularity_error(THREAD, probe);
throw_circularity_error = true;
}
}
@@ -421,7 +421,7 @@ InstanceKlass* SystemDictionary::resolve_super_or_fail(Symbol* class_name,
THROW_MSG_NULL(vmSymbols::java_lang_ClassCircularityError(), class_name->as_C_string());
}

// Resolve the super class or interface, check results on return
// Resolve the superclass or superinterface, check results on return
InstanceKlass* superk =
SystemDictionary::resolve_instance_class_or_null_helper(super_name,
class_loader,
@@ -488,8 +488,8 @@ static void double_lock_wait(JavaThread* thread, Handle lockObject) {
// For cases where the application changes threads to load classes, it
// is critical to ClassCircularity detection that we try loading
// the superclass on the new thread internally, so we do parallel
// super class loading here. This avoids deadlock for ClassCircularity
// detection for parallelCapable class loaders that lock a per-class lock.
// superclass loading here. This avoids deadlock for ClassCircularity
// detection for parallelCapable class loaders that lock on a per-class lock.
static void handle_parallel_super_load(Symbol* name,
Symbol* superclassname,
Handle class_loader,
@@ -523,12 +523,12 @@ InstanceKlass* SystemDictionary::handle_parallel_loading(JavaThread* current,
// only need check_seen_thread once, not on each loop
// 6341374 java/lang/Instrument with -Xcomp
if (oldprobe->check_seen_thread(current, PlaceholderTable::LOAD_INSTANCE)) {
log_circularity_error(oldprobe);
log_circularity_error(THREAD, oldprobe);
*throw_circularity_error = true;
return NULL;
} else {
// Wait until the first thread has finished loading this class. Also wait until all the
// threads trying to load its super class have removed their placeholders.
// threads trying to load its superclass have removed their placeholders.
while (oldprobe != NULL &&
(oldprobe->instance_load_in_progress() || oldprobe->super_load_in_progress())) {
This conversation was marked as resolved by coleenp

This comment has been minimized.

@coleenp

coleenp Mar 30, 2021
Author

This loop in handle_parallel_super_load and the one in resolve_instance_class_or_null are the same loop. The current code waits for all the LOAD_SUPER placeholders to be removed. If there are 'n' threads loading a class with a super, the first thread will add a LOAD_INSTANCE placeholder, and the second thread finding LOAD_INSTANCE will wait at the loop in resolve_instance_class_or_null for the LOAD_INSTANCE to be done.
If the first thread loads the class, and calls resolve_super_or_fail and adds the LOAD_SUPER placeholder, the next threads will call handle_parallel_super_load and wait for all the LOAD_SUPER placeholder to be removed from all the threads calling handle_parallel_super_load.
Once the first thread completes loading the super class, and removes the LOAD_SUPER placeholders, and all the other threads complete trying to load the super class, the other threads will fall into the loop in resolve_instance_class_or_null, waiting for the first thread to remove LOAD_INSTANCE.
There's only one LOAD_INSTANCE placeholder for each class/class loader pair in progress. There may be many LOAD_SUPER placeholders. The point is that both conditions can be checked in the same loop. The loop needs to wait for all LOAD_SUPERs and the main LOAD_INSTANCE to clear.
I could rename "handle_parallel_loading" to "wait_for_parallel_loading" if that makes it more clear(?)


This conversation was marked as resolved by coleenp

This comment has been minimized.

@lfoltan

lfoltan Apr 1, 2021
Member

I thought in preliminary discussions we talked about adding an assert to make sure instance_load_in_progress and super_load_in_progress couldn't both be true. Am I remembering that correctly?

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021
Author

We were talking about when instance_load_in_progress is false, then we thought we should add an assert that super_load_in_progress is also false, but super_load could still be going on for parallel threads and we have to wait for them too.
ie. I tried this and found this out. We have to wait for both conditions.

@@ -648,7 +648,7 @@ InstanceKlass* SystemDictionary::resolve_instance_class_or_null(Symbol* name,
}

// If the class is in the placeholder table with super_class set,
// handle super class loading in progress.
// handle superclass loading in progress.
if (super_load_in_progress) {
handle_parallel_super_load(name, superclassname,
class_loader,
@@ -968,7 +968,7 @@ InstanceKlass* SystemDictionary::resolve_from_stream(Symbol* class_name,

#if INCLUDE_CDS
// Load a class for boot loader from the shared spaces. This also
// forces the super class and all interfaces to be loaded.
// forces the superclass and all interfaces to be loaded.
InstanceKlass* SystemDictionary::load_shared_boot_class(Symbol* class_name,
PackageEntry* pkg_entry,
TRAPS) {
@@ -1078,7 +1078,7 @@ bool SystemDictionary::check_shared_class_super_type(InstanceKlass* klass, Insta
// super_type->class_loader_data() could be stale.
// + Don't check if loader data is NULL, ie. the super_type isn't fully loaded.
if (!super_type->is_shared_unregistered_class() && super_type->class_loader_data() != NULL) {
// Check if the super class is loaded by the current class_loader
// Check if the superclass is loaded by the current class_loader
Symbol* name = super_type->name();
InstanceKlass* check = find_instance_klass(name, class_loader, protection_domain);
if (check == super_type) {
ProTip! Use n and p to navigate between commits in a pull request.