RFR: 8259068: Streamline class loader locking

[Coleen Phillimore]

On Thu, 14 Jan 2021 23:04:54 GMT, Ioi Lam <iklam 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.
>
> src/hotspot/share/classfile/systemDictionary.cpp line 208:
> 
>> 206:       == ObjectSynchronizer::owner_other) {
>> 207:     ClassLoader::sync_nonSystemLoaderLockContentionRate()->inc();
>> 208:   }
> 
> Should the systemLoaderLockContentionRate perf counter be removed as well? I don't know what kind of compatibility risk is there. Maybe we can doit in a separate RFE.

So I think it should be done separately.  It was always zero or close to zero since nothing really contended on this lock.  I was wondering if any of these class loading perf counters are used by anything.  Something I wanted to look into afterwards.

> src/hotspot/share/classfile/systemDictionary.cpp line 1104:
> 
>> 1102:                                                      TRAPS) {
>> 1103: 
>> 1104:   assert(st != NULL, "invariant");
> 
> Is this necessary? We normally don't have asserts for non-null-ness.

I moved it from below.  Probably not necessary. The callers pass the address of a stack variable and testing this for NULL really doesn't make any sense.

> 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");
> 
> What's the reason for adding a class_loader parameter? Is it to avoid creating a new Handle?
> If there's not much performance difference, I would suggest keeping the old code to avoid code churn.

I added it to prevent a read barrier for GC, since I have the value in all the callers and to prevent creating another handle, which I also have in all the callers.

> src/hotspot/share/classfile/systemDictionaryShared.cpp line 1042:
> 
>> 1040:       Handle lockObject = compute_loader_lock_object(THREAD, class_loader);
>> 1041:       check_loader_lock_contention(THREAD, lockObject);
>> 1042:       ObjectLocker ol(lockObject, THREAD);
> 
> I think we can replace the above 3 lines with something like
> assert(!is_parallelCapable(class_loader), "ObjectLocker not required");

Yes, I agree. I updated the comment slightly also.

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

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