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,6 +322,17 @@ Klass* SystemDictionary::resolve_array_class_or_null(Symbol* class_name,
return k;
}

static inline void log_circularity_error(PlaceholderEntry* probe) {
LogTarget(Debug, class, load, placeholders) lt;
if (lt.is_enabled()) {
ResourceMark rm;
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
// during class definition to allow class circularity checking
// super-interface callers:
@@ -351,12 +362,12 @@ Klass* SystemDictionary::resolve_array_class_or_null(Symbol* class_name,
// 4. defineClass B -> calls resolve_from_stream -> calls resolve_super_or_fail for A
// resolve_super_or_fail creates placeholder for LOAD_SUPER T1 B, superclass A
// 5. resolve_or_fail for A
// (if same class loader)
// Finds placeholder for LOAD_SUPER T1 A -> throws ClassCircularityError
// Bootstrap and non-parallelCapable create placeholder LOAD_INSTANCE T1 B
// loadClass A
// if Bootstrap and non-parallelCapable class loader:
// Finds placeholder for LOAD_INSTANCE T1 A -> throws ClassCircularityError
// else
// loadClass A
// 6. defineClass A -> calls resolve_from_stream -> calls resolve_super_or_fail for B
// Finds placeholder LOAD_SUPER T1 A -> throws ClassCircularityError
// Finds placeholder LOAD_SUPER T1 A superclass B -> throws ClassCircularityError
//
// Class circularity checking for cases where classloading is delegated to
// different threads and the classloader lock is released, is described below:
@@ -418,6 +429,7 @@ InstanceKlass* SystemDictionary::resolve_super_or_fail(Symbol* class_name,
// Must check ClassCircularity before checking if super class 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);
throw_circularity_error = true;
}
}
@@ -539,12 +551,14 @@ 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);
*throw_circularity_error = true;
return NULL;
} else {
// Also wait until super class is being loaded
// 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.
while (oldprobe != NULL &&
(oldprobe->instance_load_in_progress() || oldprobe->super_load_in_progress())) {
(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.

// We only get here if the application has released the
// classloader lock when another thread was in the middle of loading a
@@ -558,12 +572,14 @@ InstanceKlass* SystemDictionary::handle_parallel_loading(JavaThread* current,
// the original thread completes the class loading or fails
// If it completes we will use the resulting InstanceKlass
// which we will find below in the systemDictionary.
oldprobe = NULL; // Other thread could delete this placeholder entry

if (lockObject.is_null()) {
SystemDictionary_lock->wait();
} else {
double_lock_wait(current, lockObject);
}

// Check if classloading completed while we were waiting
InstanceKlass* check = loader_data->dictionary()->find_class(name_hash, name);
if (check != NULL) {
@@ -730,7 +746,6 @@ InstanceKlass* SystemDictionary::resolve_instance_class_or_null(Symbol* name,
// So be careful to not exit with a CHECK_ macro between these calls.

if (loaded_class == NULL) {

// Do actual loading
loaded_class = load_instance_class(name_hash, name, class_loader, THREAD);
This conversation was marked as resolved by coleenp

This comment has been minimized.

@coleenp

coleenp Mar 30, 2021
Author

The whole passing THREAD to worry about not returning before removing the placeholder entry is a lot simpler if this code is in its own function, so I moved it there.

}
ProTip! Use n and p to navigate between commits in a pull request.