RFR: 8241234: Unify monitor enter/exit runtime entries
Yudi Zheng
yudi.zheng at oracle.com
Thu Mar 26 15:22:21 UTC 2020
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