RFR: 8259839 SystemDictionary exports too much implementation

[Ioi Lam]

On Wed, 27 Jan 2021 00:02: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.

Changes requested by iklam (Reviewer).

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?

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?

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"

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.

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

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