RFR: 8259839 SystemDictionary exports too much implementation [v2]

[David Holmes]

On Wed, 27 Jan 2021 02:04:54 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:
> 
>   return NULL for find_or_define_helper if pending exception

Hi Coleen,

I can't quite convince myself that the changes in relation to potentially parallel definition are right, but that is mainly because I can't fully reconcile the existing comments in that area. A number of comments below.

Thanks,
David

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

> 239: static Klass* handle_resolution_exception(Symbol* class_name, bool throw_error, Klass* klass, TRAPS) {
> 240:   if (HAS_PENDING_EXCEPTION) {
> 241:     assert(klass == NULL, "Should not have result with exception pending");

If klass is expected to be NULL why even bother passing it in to this function?

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

> 238: 
> 239: static Klass* handle_resolution_exception(Symbol* class_name, bool throw_error, Klass* klass, TRAPS) {
> 240:   if (HAS_PENDING_EXCEPTION) {

To me it would be clearer if the caller checked HAS_PENDING_EXCEPTION and only called handle_resolution_exception if there was one. As there are only a couple of calls this doesn't result in much code duplication.

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

> 409:     InstanceKlass* klassk = dictionary->find_class(name_hash, class_name);
> 410:     InstanceKlass* quicksuperk;
> 411:     // to support // loading: if klass done loading, just return superclass

I assume // is an approximation for || ie parallel loading? can we just say parallel please.

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

> 412:     // if super_name, & class_loader don't match:
> 413:     // if initial define, SD update will give LinkageError
> 414:     // if redefine: compare_class_versions will give HIERARCHY_CHANGED

Should these comment lines be indented because they describe what happens when super_name and loader don't match?
I find these comments are bit hard to parse in their existing form of psuedo-English.

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

> 1586: 
> 1587:   // Bootstrap and other parallel classloaders don't acquire a lock,
> 1588:   // they use placeholder token.

Unclear to me what this existing text is referring to. Parallel-capable classloaders do acquire a lock in the Java code. If this pertains only to class resolution (and so potentially skips the Java loadClass call) then the only "other parallel classloader" would be the platform-loader - right?

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

> 2030: // Verify that this placeholder exists since this class is in the middle of loading.
> 2031: void verify_placeholder(Symbol* class_name, ClassLoaderData* loader_data, bool is_parallel_capable) {
> 2032:   // Only parallel capable class loaders use placeholder table for define class (bizarre)

Is that really "Only parallel capable _built-in_ class loaders use ..."  as they by-pass the Java loadClass call that otherwise does the locking.

But don't you still need placeholder entries for class circularity errors, which can happen with non-parallel-capable classloaders too ??

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

> 2081:     }
> 2082: 
> 2083:     DEBUG_ONLY(verify_placeholder(name, loader_data, is_parallelCapable(class_loader)));

I think this would be clearer as:
 (is_parallelCapable(class_loader) verify_placeholder(name, loader_data);)```

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

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