RFR: 8262046: Clean up parallel class loading code and comments [v2]

[Coleen Phillimore]

On Sun, 28 Mar 2021 21:16:17 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>> 
>> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>> 
>> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>> 
>> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>> 
>> This updates comments to:
>> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
>> 2. move comments to be near the code they describe
>> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
>> 4. added a comment for resolve_instance_class_or_null
>> 
>> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix some comments and add placeholder CCE logging.

Added some comments and questions for reviewers.

src/hotspot/share/classfile/systemDictionary.cpp line 383:

> 381: // the super class directly, then resume steps 4-6.
> 382: //
> 383: //

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.

src/hotspot/share/classfile/systemDictionary.cpp line 393:

> 391:   // the name of the class might not be known until the stream is actually
> 392:   // parsed.
> 393:   // Bugs 4643874, 4715493

placeholders used to be added in resolve_from_stream so this comment is obsolete.

src/hotspot/share/classfile/systemDictionary.cpp line 443:

> 441:   // The instanceKlass is kept alive because the class loader is on the stack,
> 442:   // which keeps the loader_data alive, as well as all instanceKlasses in
> 443:   // the loader_data. parseClassFile adds the instanceKlass to loader_data.

This comment refers to code that is no longer here.  The placeholder entry used to keep the class alive once.

src/hotspot/share/classfile/systemDictionary.cpp line 505:

> 503: // super class loading here.
> 504: // This also is critical in cases where the original thread gets stalled
> 505: // even in non-circularity situations.

I found no evidence of why sentence at 504-505 would be true.

src/hotspot/share/classfile/systemDictionary.cpp line 561:

> 559:       // threads trying to load its super class have removed their placeholders.
> 560:       while (oldprobe != NULL &&
> 561:              (oldprobe->instance_load_in_progress() || oldprobe->super_load_in_progress())) {

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

src/hotspot/share/classfile/systemDictionary.cpp line 750:

> 748:     if (loaded_class == NULL) {
> 749:       // Do actual loading
> 750:       loaded_class = load_instance_class(name_hash, name, class_loader, THREAD);

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3200