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

Mon, 12 Feb 2018 16:42:55 -0800 

Looks good!

Thanks,
David

On 13/02/2018 6:50 AM, harold seigel wrote:

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.sei...@oracle.com <mailto:harold.sei...@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

Reply via email to