RFR: 8241234: Unify monitor enter/exit runtime entries

[coleen.phillimore at oracle.com]

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
>