RFR: 8241234: Unify monitor enter/exit runtime entries

Doug Simon doug.simon at oracle.com
Fri Mar 27 11:07:44 UTC 2020



> On 27 Mar 2020, at 11:13, Yudi Zheng <yudi.zheng at oracle.com> wrote:
> 
> Thanks David!
> 
> @Yumin, as David said, TRACE_jvmci_3 is implicitly guarded. I personally don’t have problem dropping it.
> I think back to then it was for diagnosing some lock issue.

I think that was indeed the case. If considered problematic, this tracing code can be removed.

-Doug

> 
>> On 26 Mar 2020, at 23:58, David Holmes <david.holmes at oracle.com> wrote:
>> 
>> On 27/03/2020 2:36 am, 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
>> 
>> It doesn't need to be. An explicit IF_TRACE_jcmvi is only needed to guard code that you don't want to execute unless tracing e.g. setting up the temp buffer as per the preceding code.
>> 
>>> second I think it should be removed (I am not sure what it means here).
>> 
>> It is just a logging statement showing the monitor enter has completed - it matches the previous log statement.
>> 
>> David
>> -----
>> 
>>> 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