RFR: 8241234: Unify monitor enter/exit runtime entries

[Yudi Zheng]

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
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/ <http://cr.openjdk.java.net/~yzheng/8241234/webrev.02/>

Let me know if there is any other problem.

Many thanks,
Yudi

> On 25 Mar 2020, at 15:49, 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-8061553 <https://bugs.openjdk.java.net/browse/JDK-8061553> added 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 <https://bugs.openjdk.java.net/browse/JDK-8241234>
>>>>> http://cr.openjdk.java.net/~yzheng/8241234/webrev.00/ <http://cr.openjdk.java.net/~yzheng/8241234/webrev.00/>
>>>>> 
>>>>> Many thanks,
>>>>> Yudi