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
@@ -348,37 +348,9 @@ static inline void log_circularity_error(PlaceholderEntry* probe) {
// super-class checks on its own thread to catch class circularity and
// to avoid deadlock.
//
// Take the case: A extends B extends A
//
// Single Threaded:
// 1. forName/ConstantPool reference for A: calls resolve_or_fail for A
// Bootstrap and non-parallelCapable create placeholder for LOAD_INSTANCE T1 A
// loadClass A (parallelCapable loader locks A)
// 2. defineClass A -> calls resolve_from_stream -> calls resolve_super_or_fail for B
// resolve_super_or_fail creates placeholder for LOAD_SUPER T1 A, superclass B
// 3. resolve_or_fail for B
// Bootstrap and non-parallelCapable create placeholder LOAD_INSTANCE T1 B
// loadClass B (parallelCapable loader locks B)
// 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 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 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:
//
// Step 3: If loadClass B above stalls due to ClassLoader.wait() call, or bootstrap class
// loading losing the race to acquire the SystemDictionary_lock, the second thread will attempt
// to load B to detect CCE, then waits until the first thread completes loading 4-6 and then
// resumes steps 4-6.
//
// For ParallelCapable class loaders, will call handle_parallel_super_load to attempt to load
// the super class directly, then resume steps 4-6.
// resolve_super_or_fail adds a LOAD_SUPER placeholder to the placeholder table before calling
// resolve_instance_class_or_null. ClassCircularityError is detected when a LOAD_SUPER or LOAD_INSTANCE
// placeholder for the same thread, class, classloader is found.
//
//
This conversation was marked as resolved by coleenp

This comment has been minimized.

@coleenp

coleenp Mar 30, 2021
Author

resolve_super_or_fail had this comment that I found difficult to understand and work through, which I did at one point, so I wanted to replace it with one that seemed less so. It occurs to me that this is probably unhelpful also. Maybe the whole comment from lines 351 to 381 should be replaced with a sentence like:

//resolve_super_or_fail adds a LOAD_SUPER placeholder to the placeholder table before calling
// resolve_instance_class_or_null. ClassCircularityError is detected when a LOAD_SUPER or LOAD_INSTANCE
// placeholder for the same thread, class, classloader is found.

I'm unlikely to work through the new comment example again myself.

This comment has been minimized.

@coleenp

coleenp Mar 31, 2021
Author

Since nobody wants to read this example in this comment and neither do I, I'm removing it. Debugging this using -Xlog:class+load+placeholders=debug will show the stacking of placeholders that detect CCE.

This comment has been minimized.

@iklam

iklam Mar 31, 2021
Member

I guess it's OK to remove this comment, since most of us who have looked at this code in the past few years haven't found this comment to be helpful. Maybe you can add a comment about -Xlog:class+load+placeholders=debug instead?

If we do want to improve the comment, maybe we can replacing Base->Super with something like Car->Vehicle. That way it's clear which is the supertype.

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021
Author

Thanks Ioi.

InstanceKlass* SystemDictionary::resolve_super_or_fail(Symbol* class_name,
ProTip! Use n and p to navigate between commits in a pull request.