RFR: 8259839 SystemDictionary exports too much implementation [v2]

[Coleen Phillimore]

On Wed, 27 Jan 2021 05:10:26 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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
>
> 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.

I was going to rewrite several comments in another pass.  I think the detail about redefinition isn't useful here because it's implemented somewhere else, and this code has no effect in that regard.

> 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?

This comment means that the SystemDictionary code doesn't take a lock for parallelCapable class loaders.  It uses the placeholder tokens as synchronization for parallel capable class loaders.

> 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 ??

ParallelCapable subclasses of ClassLoader also use DEFINE_CLASS placeholders because with AllowParallelDefineClass, they can define classes in parallel.  They use LOAD_INSTANCE and LOAD_SUPER placeholders to detect ClassCircularityErrors (earlier in the resolution steps).

> 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);)```

This is better.

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

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