RFR: 8241234: Unify monitor enter/exit runtime entries

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Mar 26 16:44:22 UTC 2020 


On 3/26/20 12:36 PM, Yumin Qi wrote:
> HI, Yudi and Coleen
>
> jvmciRuntime.cpp:
>
>  385 JRT_BLOCK_ENTRY(void, JVMCIRuntime::monitorenter(JavaThread* 
> thread, oopDesc* obj, BasicLock* lock))
>  386   IF_TRACE_jvmci_3 {
>  387     char type[O_BUFLEN];
>  388     obj->klass()->name()->as_C_string(type, O_BUFLEN);
>  389     markWord mark = obj->mark();
>  390     TRACE_jvmci_3("%s: entered locking slow case with obj=" 
> INTPTR_FORMAT ", type=%s, mark=" INTPTR_FORMAT ", lock=" 
> INTPTR_FORMAT, thread->name(), p2i(obj), type, mark.value(), p2i(lock));
>  391     tty->flush();
>  392   }
>  393   SharedRuntime::monitor_enter_helper(obj, lock, thread);
>  394   TRACE_jvmci_3("%s: exiting locking slow with obj=" 
> INTPTR_FORMAT, thread->name(), p2i(obj));
>  395 JRT_END
>
> line 394:
> first it is not guarded in IF_TRACE_jvmci_3
> second I think it should be removed (I am not sure what it means here).

I don't know what this jvmci code does either, but decided not to 
comment on it because it was pre-existing.   I am not sure if this code 
can access metadata like obj->klass() with the _thread_in_java state.  
Maybe it can.  It seems like something that can be investigated separately.

Coleen

>
> Thanks
> Yumin
>
> On 3/26/20 8:22 AM, Yudi Zheng wrote:
>> Thanks for the comment!
>>
>> Here is the new version: 
>> http://cr.openjdk.java.net/~yzheng/8241234/webrev.03/ 
>> <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 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/ 
>>>> <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-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
>

More information about the hotspot-compiler-dev mailing list