RFR: 8241234: Unify monitor enter/exit runtime entries
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Mar 30 15:20:48 UTC 2020
On 3/30/20 10:27 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 3/30/20 10:20 AM, Daniel D. Daugherty wrote:
>> On 3/30/20 10:15 AM, Yudi Zheng wrote:
>>> Hi Daniel,
>>>
>>> Thanks for the review! I have uploaded a new version with your
>>> comments addressed:
>>> http://cr.openjdk.java.net/~yzheng/8241234/webrev.04/
>>>
>>>> src/hotspot/share/runtime/sharedRuntime.hpp
>>>> Please don't forget to update the copyright year before you push.
>>> Fixed.
>>>
>>>> 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.
>>> Also, I noticed that C2 was using CHECK
>>>> ObjectSynchronizer::enter(h_obj, lock, CHECK);
>>> While C1 and JVMCI were using THREAD:
>>>> ObjectSynchronizer::enter(h_obj, lock->lock(), THREAD);
>>> I have no idea when to use what, and hope unifying to the C2 entries
>>> would help.
>>> Let me know if there is something I should address in this patch.
>>> Otherwise, I would
>>> rather leave it to the expert, i.e., you ;)
>>
>> Yes, please leave it for me to clean up.
>
> It appears that both of these places (monitor_enter_helper and
> monitor_exit_helper) should be using CATCH.
Here's CATCH:
src/hotspot/share/utilities/exceptions.hpp:
// The CATCH macro checks that no exception has been thrown by a
function; it is used at
// call sites about which is statically known that the callee cannot
throw an exception
// even though it is declared with TRAPS.
#define CATCH \
THREAD); if (HAS_PENDING_EXCEPTION) { \
oop ex = PENDING_EXCEPTION; \
CLEAR_PENDING_EXCEPTION; \
ex->print(); \
ShouldNotReachHere(); \
} (void)(0
I don't think you want either monitor_enter_helper() or
monitor_exit_helper() crashing the VM via a ShouldNotReachHere()
if someone happens to throw an exception at a thread during
a 'monitorenter' or 'monitorexit' bytecode.
What am I missing here?
Dan
>
> Coleen
>>
>>
>>>> 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
>>> I assume I don’t need to do anything regarding the comments above.
>>
>> Correct. Just observations on the old code.
>>
>>
>>>> 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.
>>> I removed all the tracing code.
>>
>> Thanks for cleaning that up.
>>
>> Dan
>>
>>>
>>> Many thanks,
>>> Yudi
>>
>
More information about the hotspot-compiler-dev
mailing list