RFR: 8212682: Avoid holding Compile_lock when blocking for GC in ObjArrayKlass::allocate_objArray_klass()

[Erik Österlund]

Hi Coleen,

On 2018-10-23 20:51, coleen.phillimore at oracle.com wrote:
>
> I wonder if the Compile_lock is added for this in ci/ciEnv.cpp (and 
> it's copy in jvmtiEnv.cpp):
>
> ciKlass* ciEnv::get_klass_by_name_impl(ciKlass* accessing_klass,
> ...
>     MutexLocker ml(Compile_lock);
>     Klass* kls;
>     if (!require_local) {
>       kls = 
> SystemDictionary::find_constrained_instance_or_array_klass(sym, loader,
> KILL_COMPILE_ON_FATAL_(fail_type));
>     } else {
>       kls = SystemDictionary::find_instance_or_array_klass(sym, 
> loader, domain,
> KILL_COMPILE_ON_FATAL_(fail_type));
>     }
>     found_klass = kls;
>   }
>
> which eventually calls in SystemDictionary::
>
>     if (k != NULL) {
>       k = k->array_klass_or_null(fd.dimension());
>     }
>

If that path was ever taken before, it seems to me that would result in 
a deadlock with the existing code, if allocating the array klass is 
necessary, as the Compile_lock is not conditionally taken in 
array_klass_impl. So if the callers you point out already have the 
Compile_lock, it would just hang, and in debug mode, it would assert 
that the lock is already held. So it seems like already today, we rely 
on that path never being taken for correctness.

>
> And maybe the Compile_lock keeps the Klass::_higher_dimension have a 
> consistent value.  Maybe the code in ciEnv.cpp and jvmciEnv.cpp should 
> take MultiArray_lock instead.  The problem is that Compile_lock 
> protects too many things.

I have gone through all callers of higher_dimension() using rtags, and 
did not find anything suggesting the Compile_lock keeps it consistent. 
The callers are either safepoint operations, *::array_klass_impl itself, 
or "protected" by another lock (VtableStats::compute is protected by the 
CLDG_lock only, which is not being used to protect the higher or lower 
dimensions, so this code is possibly wrong, but only called when using 
-XX:+PrintVtableStats).

> I don't know if this is safe to remove unfortunately.

Hope it feels safer now.

Thanks,
/Erik

> Coleen
>
>
> On 10/23/18 10:59 AM, Erik Österlund wrote:
>> Hi,
>>
>> We occasionally blockingly wait for GC in allocations in 
>> ObjArrayKlass::allocate_objArray_klass(), while holding the 
>> Compile_lock.
>> This is problematic for concurrent class unloading that needs to hold 
>> that lock in the unloading path. This introduces a potential deadlock 
>> situation.
>>
>> After staring enough at this code together with help from Coleen, I 
>> have convinced myself that the Compile_lock is in fact not needed in 
>> this path, and there is nothing to my knowledge it actually protects 
>> here. The vague comment next to the lock suggests its purpose is to 
>> protect vtable stubs. But it doesn't. There is a VtableStubs_lock for 
>> that, and I find no traces if the Compile_lock having anything to do 
>> with that.
>>
>> Coleen helped me trying to trace down where this locking code came 
>> from. It takes us back to before anyone was around, and does not seem 
>> to have changed in the past 2 decades, and its origins are a bit 
>> unclear. The theory is that perhaps vtable stubs used to be protected 
>> by the Compile_lock in ancient times, and the locking code is still 
>> around because nobody dared to touch it.
>>
>> Since both code analysis and mach5 suggest there is no reason for 
>> this lock to be held here, I propose to remove it with this patch.
>>
>> If anyone that has been around for more than 2 decades in HotSpot 
>> happens to know the reason why this locking was introduced, any 
>> commentary around that would be very appreciated.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8212682/webrev.00/
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8212682
>>
>> Thanks,
>> /Erik
>