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

[coleen.phillimore at oracle.com]

On 10/24/18 4:18 AM, Erik Österlund wrote:
> 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.

Yes, thank you for the explanation.  I trust rtags better than my 
eyeballs.  Thanks to Dean for the archeology!

Looks good!
Coleen

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