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

[Karen Kinnear]

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