RFR: 8259068: Streamline class loader locking

[Daniel D.Daugherty]

On Wed, 13 Jan 2021 23:30:09 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> The system_loader_lock_object is never actually acquired when loading a class with a parallelCapable class loader, which includes the bootloader (class_loader == NULL), except in one place before restore_unshareable_info is called.  In this case, the per-class lock (BuiltinLoader.getClassLoadingLock) is held or the placeholder for LOAD_INSTANCE is present which implements mutual exclusion.  Ioi and I separately verified this while chasing down another bug.
> This change removes the system_loader_lock_obj and extraneous code around compute_loader_lock_object to make it consistent.  This also removes the bool argument to ObjectLocker.  If the oop passed into ObjectLocker is null, we don't lock the object, which is consistent with the Mutex class.  This change also passes the class_loader to define_class to save an OopStorage.resolve() call.
> Tested with tier1-8.

Thumbs up. I only have minor nits and wording suggestions.
It would be good if @iklam had a chance to review this also.

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

> 577:   assert_lock_strong(SystemDictionary_lock);
> 578: 
> 579:   assert(lockObject() != NULL, "unexpected double_lock_wait");

Perhaps a different assert string:
    "lockObject must be non-NULL"

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

> 580:   bool calledholdinglock
> 581:       = ObjectSynchronizer::current_thread_holds_lock(thread->as_Java_thread(), lockObject);
> 582:   assert(calledholdinglock,"must hold lock for notify");

L582 nit - s/,/, /  (not your bug)

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

> 581:       = ObjectSynchronizer::current_thread_holds_lock(thread->as_Java_thread(), lockObject);
> 582:   assert(calledholdinglock,"must hold lock for notify");
> 583:   assert(!is_parallelCapable(lockObject), "unexpected double_lock_wait");

Perhaps a different assert string:
"lockObject must not be parallelCapable"

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

> 750:   }
> 751: 
> 752:   assert(placeholders()->compute_hash(name) == name_hash, "they're the same hashcode");

Why delete this assert?

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

> 1102:                                                      TRAPS) {
> 1103: 
> 1104:   assert(st != NULL, "invariant");

Thanks for moving this assert here.

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

> 1633:   HandleMark hm(THREAD);
> 1634:   ClassLoaderData* loader_data = k->class_loader_data();
> 1635:   assert(loader_data->class_loader() == class_loader(), "they are the same");

Perhaps:
s/they are the same/they must be the same/

src/hotspot/share/classfile/systemDictionaryShared.cpp line 1034:

> 1032:       Dictionary* dictionary = loader_data->dictionary();
> 1033: 
> 1034:       unsigned int d_hash = dictionary->compute_hash(name);

Is there a reason that `d_hash` was previously computed
outside the `SystemDictionary_lock`?

src/hotspot/share/runtime/synchronizer.hpp line 189:

> 187:   // reenter reclaims lock with original recursion count
> 188:   intx complete_exit(TRAPS)  { return ObjectSynchronizer::complete_exit(_obj, THREAD); }
> 189:   void reenter(intx recursions, TRAPS)  { ObjectSynchronizer::reenter(_obj, recursions, CHECK); }

Nice cleanup.

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

Marked as reviewed by dcubed (Reviewer).

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