RFR: 8241234: Unify monitor enter/exit runtime entries

[coleen.phillimore at oracle.com]

This looks good to me.
Thank you.
Coleen

On 3/26/20 11:22 AM, Yudi Zheng wrote:
> Thanks for the comment!
>
> Here is the new version: 
> http://cr.openjdk.java.net/~yzheng/8241234/webrev.03/
>
> Differences w.r.t. webrev.00 are that:
> 1. parameter _obj is now renamed to obj
> 2. oop obj(_obj) is removed
>
> Many thanks,
> Yudi
>
>
>> On 26 Mar 2020, at 15:25, coleen.phillimore at oracle.com 
>> <mailto:coleen.phillimore at oracle.com> wrote:
>>
>>
>>
>> On 3/26/20 9:48 AM, Yudi Zheng wrote:
>>> Hi Coleen,
>>>
>>> The monitor exit entries are marked with JRT_LEAF which contains
>>> >  debug_only(NoHandleMark __hm;)                                     \
>>>
>>> This disallows the creation of Handle within the monitor exit entries.
>>> > # A fatal error has been detected by the Java Runtime Environment:
>>> > #
>>> > #  Internal Error (open/src/hotspot/share/runtime/handles.cpp:35), pid=28631, tid=28687
>>> > #  assert(_no_handle_mark_nesting == 0) failed: allocating handle inside NoHandleMark
>>
>> You are right.  Thanks for pointing this out.
>>> I have uploaded a version that addresses the following comments:
>>> 1. Have monitor_enter_helper take Handle as arguments.
>>> 2. Avoid using _obj as parameter name as it indicates a field 
>>> according to the coding conventions.
>>> 3. oop obj(_obj); is not necessary as there is an implicit 
>>> conversion from oopDesc* to oop
>>>
>>> See http://cr.openjdk.java.net/~yzheng/8241234/webrev.02/
>>>
>>
>> Actually, I apologize that I think I am wrong about making 
>> monitor_enter_helper take a Handle because I don't think you can 
>> create Handle from a thread that isn't _thread_in_vm.
>>
>> +// Handles the uncommon case in locking, i.e., contention or an 
>> inflated lock.
>> +JRT_BLOCK_ENTRY(void, 
>> SharedRuntime::complete_monitor_locking_C(oopDesc* obj, BasicLock* 
>> lock, JavaThread* thread))
>> + Handle h_obj(thread, obj);
>> + SharedRuntime::monitor_enter_helper(h_obj, lock, thread);
>> +JRT_END
>>   
>> So I think SharedRuntime::monitor_enter_helper() shouldn't take a 
>> Handle and that would allow all paths to have this fast path:
>>
>>     if (!SafepointSynchronize::is_synchronizing()) {
>>       // Only try quick_enter() if we're not trying to reach a safepoint
>>       // so that the calling thread reaches the safepoint more quickly.
>> -    if (ObjectSynchronizer::quick_enter(_obj, thread, lock)) return;
>> +    if (ObjectSynchronizer::quick_enter(h_obj(), thread, lock)) return;
>>     }
>>
>> Then create the handle in the JRT_BLOCK_NO_ASYNC where it was. Maybe 
>> this gets you back to your original webrev.  Sorry about this.
>>
>> Thanks,
>> Coleen
>>
>>> Let me know if there is any other problem.
>>>
>>> Many thanks,
>>> Yudi
>>>
>>>> On 25 Mar 2020, at 15:49, coleen.phillimore at oracle.com 
>>>> <mailto:coleen.phillimore at oracle.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/24/20 1:14 PM, Yudi Zheng wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Thanks for the review! Sorry I missed your email. And thanks 
>>>>> Vladimir for the reminder.
>>>>>
>>>>>>> I think you should have monitor_enter_helper and maybe 
>>>>>>> monitor_exi_helper both take Handle as arguments.  This looks 
>>>>>>> very strange.
>>>>>
>>>>> Shall we change ObjectSynchronizer::exit to accept Handle as 
>>>>> argument as well?
>>>>
>>>> I think you should.
>>>>>
>>>>>>> +void SharedRuntime::monitor_enter_helper(oopDesc* _obj, 
>>>>>>> BasicLock* lock, JavaThread* thread) {
>>>>>>> Because it creates a Handle anyway.  We try to eagerly pass 
>>>>>>> Handle so that we don't have to search through the code to make 
>>>>>>> sure there aren't any unhandled oops.
>>>>>>> The _obj parameter name doesn't follow coding conventions (I was 
>>>>>>> looking for it to be a field in something).
>>>>>
>>>>> I took the parameter name from the original methods:
>>>>> JRT_BLOCK_ENTRY(void, 
>>>>> SharedRuntime::complete_monitor_locking_C(oopDesc* _obj, 
>>>>> BasicLock* lock, JavaThread* thread))
>>>>> JRT_LEAF(void, 
>>>>> SharedRuntime::complete_monitor_unlocking_C(oopDesc* _obj, 
>>>>> BasicLock* lock, JavaThread * THREAD))
>>>>> Will Change that.
>>>>>
>>>>>>> + oop obj(_obj);
>>>>>>> Is there not already a implicit conversion from oopDesc* to oop?
>>>>>
>>>>> Also took from the original methods. Will get rid of that.
>>>>
>>>> Great. thank you.
>>>> Coleen
>>>>>
>>>>> Thanks,
>>>>> Yudi
>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>> On 3/20/20 11:03 AM, Yudi Zheng wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review this patch that unifies the monitor enter/exit 
>>>>>>>> runtime entries used by C1, C2, and JVMCI.
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8061553added support 
>>>>>>>> for faster "slow" locking paths.
>>>>>>>> These paths are not currently used by JVMCI (or C1) leading to 
>>>>>>>> slower performance on programs
>>>>>>>> that have a lot of short lived locked sections (such as dacapo:h2).
>>>>>>>>
>>>>>>>> This patch creates helper methods based on C2’s monitor 
>>>>>>>> enter/exit entries, and shares with the C1,
>>>>>>>> JVMCI runtimes. It will prevent these runtimes from missing 
>>>>>>>> future updates.
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8241234
>>>>>>>> http://cr.openjdk.java.net/~yzheng/8241234/webrev.00/
>>>>>>>>
>>>>>>>> Many thanks,
>>>>>>>> Yudi
>>>
>>
>