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

[Erik Osterlund]

Hi Dean,

Thanks for the review!

/Erik

> On 19 Nov 2018, at 19:26, dean.long at oracle.com wrote:
> 
> Yes, Reviewed!
> 
> dl
> 
>> On 11/19/18 6:09 AM, Erik Österlund wrote:
>> Hi Dean,
>> 
>> May I interpret your commentary as Reviewed?
>> 
>> Thanks,
>> /Erik
>> 
>>> On 2018-10-24 22:20, dean.long at oracle.com wrote:
>>>> On 10/24/18 1:52 AM, Erik Österlund wrote:
>>>> Hi Dean,
>>>> 
>>>>> On 2018-10-23 22:27, dean.long at oracle.com wrote:
>>>>> Can allocate_objArray_klass() end up calling SystemDictionary::add_to_hierarchy()?  If so, then you'll end up locking Compile_lock anway, but in the wrong order with respect to MultiArray_lock.
>>>> 
>>>> No, I can't see that allocate_objArray_klass() ever calls SystemDictionary::add_to_hierarchy(). If it did, we would assert that the Compile_lock is held; it never re-acquires the lock.
>>>> 
>>> 
>>> OK, I forgot that mutex's can't be locked recursively.
>>> 
>>> I found a clue to the reason for original 1998 change.  This was also added at the same time, to klassVtable::initialize_vtable():
>>> 
>>>   // We need the compile lock in order to force the super vtables and
>>>   // methodOop code pointers to stay constant.
>>>   // (Race scenario: compiler thread updates a vtable entry while we're copying
>>>   // down entries from the superclass vtable.)
>>>   assert_lock(Compile_lock);
>>> 
>>> By 1999, the vtable code changed a lot and the assert above went away, so it was probably just an oversight that the extra uses of Compile_lock weren't removed at the same time.
>>> 
>>> dl
>>> 
>>>> Thanks,
>>>> /Erik
>>>> 
>>>>> dl
>>>>> 
>>>>>> On 10/23/18 7: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
>>>>> 
>>>> 
>>> 
>> 
>