RFR: 8241234: Unify monitor enter/exit runtime entries

Fri Mar 27 16:21:36 UTC 2020

Hi Yudi,

On 3/26/20 11: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/>

src/hotspot/share/runtime/sharedRuntime.hpp
     Please don't forget to update the copyright year before you push.

src/hotspot/share/runtime/sharedRuntime.cpp
     L2104:   ObjectSynchronizer::exit(obj, lock, THREAD);
         The use of 'THREAD' here and 'TRAPS' in the function itself
         standout more now, but that's something for me to cleanup.

src/hotspot/share/c1/c1_Runtime1.cpp
     old L718:   assert(thread == JavaThread::current(), "threads must 
correspond");
         Removed in favor of the assert in 
SharedRuntime::monitor_enter_helper().
         Okay that makes sense.

     old L721:   EXCEPTION_MARK;
         Removed in favor of the same in 
SharedRuntime::monitor_enter_helper().
         Okay that makes sense.

src/hotspot/share/jvmci/jvmciRuntime.cpp
     old L403:   assert(thread == JavaThread::current(), "threads must 
correspond");
     old L406:   EXCEPTION_MARK;
         Same as for c1_Runtime1.cpp

     L390:     TRACE_jvmci_3("%s: entered locking slow case with obj="...
     L394:   TRACE_jvmci_3("%s: exiting locking slow with obj="
     L417:     TRACE_jvmci_3("%s: exited locking slow case with obj="
         But this is no longer the "slow" case so I'm a bit confused.

         Update: I see there's a comment about the tracing being removed.
         I have no opinion on that since it is JVM/CI code, but the word
         "slow" needs to be adjusted if you keep it.

Thumbs up!

Dan

P.S.
For those reviewers tracking Async Monitor Deflation (8153224)...
Yes, I'll have to make small updates to adjust.

>
> 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