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

[Erik Österlund]

Hi Coleen,

Thanks for the review!

/Erik

On 2018-10-31 16:44, coleen.phillimore at oracle.com wrote:
> Looks good!
> Coleen
>
> On 10/31/18 9:16 AM, Erik Österlund wrote:
>> Hi Coleen,
>>
>> Thanks for the review.
>>
>> New full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8212681/webrev.01/
>>
>> Incremental:
>> http://cr.openjdk.java.net/~eosterlund/8212681/webrev.00_01/
>>
>> Fixed Robbin's copyright headers, Aleksey's AArch64 build aaaaand...
>>
>> On 2018-10-30 23:32, coleen.phillimore at oracle.com wrote:
>>>
>>> Erik,
>>>
>>> It looks like you've removed the only use of VerifyMutexLocker. I 
>>> was going to ask if you ran with logging that called 
>>> nmethod::print_calls() but I don't see any callers. Maybe 
>>> nmethod::print_nmethod() should call it.  Can you experiment with 
>>> calling this print_calls to see if you can remove VerifyMutexLocker?
>>
>> Removed.
>>
>>> http://cr.openjdk.java.net/~eosterlund/8212681/webrev.00/src/hotspot/share/runtime/sweeper.cpp.frames.html 
>>>
>>>
>>> 676 MutexLockerEx mex(CompiledIC_lock);
>>>
>>>
>>> Why isn't this CompiledICLocker (nm)?  Can youy add a comment why 
>>> it's different?
>>
>> Good catch. Should really use the CompiledICLocker, so I changed it 
>> so it does that.
>>
>>> http://cr.openjdk.java.net/~eosterlund/8212681/webrev.00/src/hotspot/share/runtime/vmBehaviours.new.hpp.html 
>>>
>>>
>>> I see you've added more (funny spelling) behaviours.  I think this 
>>> one should go in the code directory with the user of it and be 
>>> called compiledICBehaviours.hpp or even codeBehaviours.hpp.
>>
>> Changed to code/codeBehaviours.hpp as requested.
>>
>> Thanks,
>> /Erik
>>
>>> thanks,
>>> Coleen
>>>
>>> On 10/23/18 9:03 AM, Erik Österlund wrote:
>>>> Hi Robbin,
>>>>
>>>> Thanks for the review.
>>>>
>>>> /Erik
>>>>
>>>> On 2018-10-23 14:50, Robbin Ehn wrote:
>>>>> Hi Erik,
>>>>>
>>>>> Looks better than before :), don't forget copyright years before 
>>>>> push.
>>>>>
>>>>> /Robbin
>>>>>
>>>>> On 10/22/18 12: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
>>>>
>>>
>>
>