RFR: 8212681: Refactor IC locking to use a fine grained CompiledICLocker

[David Holmes]

Hi Erik,

On 22/10/2018 9:48 PM, Erik Österlund wrote:
> Hi David,
> 
> With concurrent class unloading, there is a possibility that two 
> JavaThreads both miss an IC, run the 
> SharedRuntime::handle_ic_miss_helper, lock disjoint fine grained IC 
> locks, both ask if a CompiledIC::is_megamorphic(), causing the 
> VtableStubs_lock in VtableStubs::entry_point to become contended, and 
> hence possibly rely on the VM thread using lock sneaking to not cause a 
> deadlock.

That was my concern. We might go from a situation where sneaking is so 
rare that we can't observe it, to one where it is more common.

> If we want to get rid of sneaky locking, it seems like we should use 
> MutexLockerEx and conditionally lock VtableStubs_lock only when we are 
> not the VM thread in a safepoint. It seems kind of evil to build the 
> correctness of a solution based on redundant locking on a higher level 
> that removes the contention on another lower level lock that would if 
> contended cause a deadlock.

It's a usage protocol that allows us to show certain locks are immune 
from sneaking - which can be seen as a good thing to know at least until 
sneaking is removed, or which might allow sneaking to be turned off.

But arguably it also shows that the lower-level locks are redundant and 
we should be able to rely on the higher-level lock only (at least prior 
to your change.)

> I think the InlineCacheBuffer_lock should be okay, because in the paths 
> that grab that lock, I make sure to take the CompiledIC_lock first, with 
> or without fine grained IC locking.

So you hold the CompiledIC_lock, a fine-grained IC lock and the 
InlineCacheBuffer_lock all at the same time? That seems somewhat 
excessive. :)

Thanks,
David

> Thanks,
> /Erik
> 
> On 2018-10-22 13:22, David Holmes wrote:
>> Hi Erik,
>>
>> The CompiledIC_lock also "guards" use of other nested locks and 
>> thereby avoids the possibility of lock sneaking on those nested locks 
>> [1]. Will your concurrent unloading changes change this?
>>
>> Thanks,
>> David
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8210832
>>
>> On 22/10/2018 8:59 PM, Erik Österlund wrote:
>>> Hi,
>>>
>>> Today, one must acquire the global CompiledIC_lock to have permission 
>>> to perform certain IC updating activities. The lock is used outside 
>>> of safepoints, but for example during nmethod unloading due to GC, 
>>> lock free IC cache cleaning is performed, which is safe due to being 
>>> in a safepoint.
>>>
>>> With concurrent class unloading introduced, the global lock is too 
>>> coarse grained, and a per-nmethod mechanism is needed instead. In 
>>> order to allow this, an abstract CompiledICLocker class could help 
>>> ensuring the safety of IC caches, and allow both the fine grained 
>>> (but density costly) approach for users of concurrent class 
>>> unloading, and resort to the default global locking approach otherwise.
>>>
>>> The idea is to retain the coarse grained implementation of the 
>>> synchronization when concurrent class unloading is not being used, as 
>>> supporting fine grained locking could be more costly in memory, and 
>>> there does not seem to be any use in doing this unless concurrent 
>>> class unloading is being used.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8212681/webrev.00/
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8212681
>>>
>>> Thanks,
>>> /Erik
>