RFR: 8241234: Unify monitor enter/exit runtime entries

David Holmes david.holmes at oracle.com
Thu Mar 26 22:58:29 UTC 2020 

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