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

[Lois Foltan]

On Thu, 1 Apr 2021 03:11:07 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:
> 
>   Add comment about logging placeholders.

I think this is a good clean up to consolidate the 2 while loops Coleen into SystemDictionary::handle_parallel_loading().  A couple of minor review comments.

Thanks,
Lois

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

> 327:   if (lt.is_enabled()) {
> 328:     ResourceMark rm;
> 329:     LogStream ls(lt);

Is it helpful to pass THREAD in for the ResourceMark?

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

> 339: //    parse_interfaces - from defineClass
> 340: // super-class callers:
> 341: //   ClassFileParser - from defineClass

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

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

> 490: // the superclass on the new thread internally, so we do parallel
> 491: // super class loading here.  This avoids deadlock for ClassCircularity
> 492: // detection for parallelCapable class loaders that lock a per-class lock.

"lock on a per-class lock"

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

> 532:       while (oldprobe != NULL &&
> 533:              (oldprobe->instance_load_in_progress() || oldprobe->super_load_in_progress())) {
> 534: 

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?

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

> 784:           if (JvmtiExport::should_post_class_load()) {
> 785:             JvmtiExport::post_class_load(THREAD->as_Java_thread(), loaded_class);
> 786:           }

This seems to be all now handled in load_instance_class, correct?

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

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