RFR: 8259839 SystemDictionary exports too much implementation [v3]

[David Holmes]

On Wed, 27 Jan 2021 12:11: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 two additional commits since the last revision:
> 
>  - Fix comments and verify_placeholder
>  - Fix handle_resolution_exception to make more sense.

Thanks for the updates. Further comments below.

Also there is a comment block at line 759 that says 5 cases ... but only lists 4.

Thanks,
David

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

> 267:   if (HAS_PENDING_EXCEPTION || klass == NULL) {
> 268:     handle_resolution_exception(class_name, throw_error, CHECK_NULL);
> 269:   }

I find the logic for this very confusing. If klass == NULL then isn't it the case that there must be a pending exception?
Also, looking inside handle_resolution_exception, if the pending exception is not a ClassNotFoundException then we completely ignore it and throw either ClassNotFoundException or NoClassDefFoundError. I would have expected to at least chain the original exception.

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

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

Maybe I'm having trouble following the call sequence, but non-parallel capable loaders also add placeholder entries, and then call check_constraints. So why do we not need to verify the placeholder in that case?

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

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