RFR: 8259839 SystemDictionary exports too much implementation

[Coleen Phillimore]

On Wed, 27 Jan 2021 00:42:23 GMT, Ioi Lam <iklam 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.
>
> src/hotspot/share/classfile/systemDictionary.cpp line 1748:
> 
>> 1746:     k->class_loader_data()->add_to_deallocate_list(k);
>> 1747:   } else if (HAS_PENDING_EXCEPTION) {
>> 1748:     assert(defined_k != NULL, "Should not have a klass if there's an exception");
> 
> Should the assert be `defined_k == NULL`? I wonder if we have a test case that covers this branch.

Yes, it should be.  That's a cut/paste error on my part.  There is a test for this: runtime/Metaspace/DefineClass.java

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

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