RFR: 8259839 SystemDictionary exports too much implementation [v6]

[David Holmes]

On Thu, 28 Jan 2021 23:42:55 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> I apologize for the large change but it's really a bunch of little cleanups combined made while trying to understand the code.  Only number 8 below changes any behavior.  Tested with tier1-8 (24hr Dacapo is still running).
>> 
>> In systemDictionary.cpp:
>>  1. moved placeholder table and other table sizes into systemDictionary.cpp
>>  2. Made SystemDictionary::find_class() into a verification function called verify_dictionary_entry since that's what it was used for 
>>  3. Moved a resolve_or_fail() down and removed redundant checks before calling handle_resolution_exception
>>  4. Fixed some comments
>>  5. child_name confused me. class_name and super_name are more meaningful in these contexts
>>  6. callers of find_or_define_instance_class had to check for a different klass return (with AllowParallelDefineClass), or an exception in order to deallocate one of the classes, so I wrapped it into a function and renamed find_or_define_instance_class into find_or_define_helper.
>>  7. find_placeholder() is really used to verify_placeholder(). Actually the verification was a nop. Fixing it found a bug in that CDS was calling define_instance_class rather than find_or_define_instance_class for parallelCapable class loader's that it shares, so verify_placeholder() was failing.  Changed the CDS code to call find_or_define_instance_class instead.
>> 
>> In systemDictionary.hpp, I moved the static data members and moved declarations around to make private and have a single protected block for SystemDictionaryShared to use.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix more comments.

Hi Coleen,

A few more follow up comments below.

I think things are generally okay - and any concerns may just be my misunderstanding.

Thanks,
David

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

> 243:       THROW_MSG_CAUSE(vmSymbols::java_lang_NoClassDefFoundError(), class_name->as_C_string(), e);
> 244:     } else {
> 245:       return; // the caller will throw the incoming exception

Thanks for fixing this. It seems we must have some missing test coverage here though.

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

> 782:             throw_circularity_error = true;
> 783:           } else {
> 784:             // case 3: traditional: should never see load_in_progress.

It seems odd to hit case 3 first in the code. I think the comment ordering of the cases should be retained.

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

> 790:                 SystemDictionary_lock->wait();
> 791:               } else {
> 792:               // case 3: traditional with broken classloader lock. wait on first

Why combine these into case 3 instead of keeping two distinct cases as per the original comment?

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

Marked as reviewed by dholmes (Reviewer).

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