RFR: 8241234: Unify monitor enter/exit runtime entries

[coleen.phillimore at oracle.com]

On 3/30/20 11:20 AM, Daniel D. Daugherty wrote:
> 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.

Of course, it's more complicated.  I was looking at this:

    ObjectSynchronizer::enter(h_obj, lock, CHECK);
    assert(!HAS_PENDING_EXCEPTION, "Should have no exception here");
    JRT_BLOCK_END

Which essentially wants to, but fails to, assert that there's no 
exceptions possible (it is enclosed with JRT_BLOCK_NO_ASYNC).

I can't see whether ObjectSynchronizer::enter() has code to exclude 
async exceptions from being installed (possibly biased locking revoking 
at a handshake?).

And this:

+void SharedRuntime::monitor_exit_helper(oopDesc* obj, BasicLock* lock, 
JavaThread* thread) {
+ assert(JavaThread::current() == thread, "invariant");
      // Exit must be non-blocking, and therefore no exceptions can be thrown.
      EXCEPTION_MARK;
      ObjectSynchronizer::exit(obj, lock, THREAD);
- }
+}

Which has ExceptionMark whose destructor has:

fatal("ExceptionMark destructor expects no pending exceptions");

Which appears to be enabled in production.

So either way, you want the functionality of CHECK, just not have it 
assert in production (which was a surprise to me).

Actually, I agree with you that you want to clean this up.

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