RFR: 8259839 SystemDictionary exports too much implementation

[Coleen Phillimore]

On Wed, 27 Jan 2021 00:25:05 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/systemDictionaryShared.cpp line 1219:
> 
>> 1217: // java/lang/Object id: 0
>> 1218: // Interface   id: 2 super: 0 source: cust.jar
>> 1219: // KlassClass  id: 4 super: 0 interfaces: 2 source: cust.jar
> 
> `KlassClass` sounds like it's a "class of classes" which would be misleading in this context. How about `MyClass` instead?

How about SharedClass ?

> src/hotspot/share/classfile/systemDictionary.cpp line 274:
> 
>> 272:   // Check for pending exception or null klass, and throw exception
>> 273:   return handle_resolution_exception(class_name, throw_error, klass, THREAD);
>> 274: }
> 
> How about moving this block of code to its old location to minimize the diff?

I moved that resolve_or_fail to be with the rest of the resolve_or_fail, and the resolve_or_nulls.

> src/hotspot/share/classfile/systemDictionary.cpp line 1124:
> 
>> 1122:   // Add class just loaded
>> 1123:   // If a class loader supports parallel classloading, handle parallel define requests.
>> 1124:   // find_or_define_instance_class may return a different InstanceKlass
> 
> How about adding "// find_or_define_instance_class may return a different InstanceKlass, in which case the old k would be deallocated"

Ok.

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

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