RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

[harold seigel]

Hi Karen,

Thanks for looking at this!

I re-ran the ParallelClassLoading tests with no options, with 
-XX:+AllowParallelDefineClass, and with -XX:+AlwaysLockClassLoader, and 
all the tests passed.  I also determined that the few Mach5 regression 
test failures that I encountered were unrelated to my change.

Please see this latest webrev that adds 12 as an expiration date, 
removes the 'guarantee' section, and fixes the comment at line 786 of 
systemDictionary.cpp.

    http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html

Thanks, Harold

On 2/12/2018 2:39 PM, Karen Kinnear wrote:
> Harold,
>
> Thanks for doing this.
>
> I think you told me that
> 1) the version change has made it in
> 2) you also put 12 as an expiration date
> 3) you are running the ParallelClassLoading tests with the remaining 
> two flags (you’ve already
> run them without any flags):
>   AllowParallelDefineClass = true and AlwaysLockClassLoader=true
>
> In terms of the guarantee in question
> // For UnsyncloadClass only
>   848       // If they got a linkageError, check if a parallel class load succeeded.
>   849       // If it did, then for bytecode resolution the specification requires
>   850       // that we return the same result we did for the other thread, i.e. the
>   851       // successfully loaded InstanceKlass
>   852       // Should not get here for classloaders that support parallelism
>   853       // with the new cleaner mechanism, even with AllowParallelDefineClass
>   854       // Bootstrap goes through here to allow for an extra guarantee check
> 855 if (UnsyncloadClass || (class_loader.is_null())) {
>   856         if (k == NULL && HAS_PENDING_EXCEPTION
>   857           && PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
>   858           MutexLocker mu(SystemDictionary_lock, THREAD);
>   859           InstanceKlass* check = find_class(d_hash, name, dictionary);
>   860           if (check != NULL) {
>   861             // Klass is already loaded, so just use it
>   862             k = check;
>   863             CLEAR_PENDING_EXCEPTION;
>   864             guarantee((!class_loader.is_null()), "dup definition for bootstrap loader?");
>   865           }
>   866         }
>   867       }
>
>   1) I agree you can remove the entire section
>       - the guarantee was there for future proofing in case we ever 
> allowed parallel class loading of the
>         same class for the null loader and to make sure I didn’t have 
> any logic holes.
>       - I would not put an assertion for the first half of the 
> condition - I would remove completely
>       - the code currently prevents parallel class loading of the same 
> class for the null loader at:
> resolve_instance_class_or_null see line 785 …
>   while (!class_has_been_loaded && old probe && old 
> probe->instance_load_in_progress()) {
>      //  case x: bootstrap class loader: prevent futile class loading,
>      // wait on first requestor
>      if (class_loader.is_null()) {
>         SystemDictionary_lock->wait();
>
> This logic means that there is a registered INSTANCE_LOAD on this 
> placeholder entry.
>
>
> Other minor comments (sorry if you already got these and I missed them 
> in earlier emails)
> - all in SystemDictionary.cpp
>
> 1. line 72 comment “Five cases:” -> “Four cases:”
> So you removed case 3 and renumbered, so old references to case 4 -> 
> case 3 ,and old references
> to case 5 become case 4:
>
> So - line 786, “Case 4” -> “case 3”
>
> thanks,
> Karen
>
>
>> On Feb 12, 2018, at 11:13 AM, harold seigel <harold.seigel at oracle.com 
>> <mailto:harold.seigel at oracle.com>> wrote:
>>
>> Hi Alan,
>>
>> Thanks for looking at this.
>>
>> Harold
>>
>> On 2/12/2018 2:52 AM, Alan Bateman wrote:
>>> On 12/02/2018 06:54, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> Adding core-libs-dev so they are aware of the ClassLoader change.
>>> Thanks, that part is okay and good to see this going away.
>>>
>>> -Alan
>>
>