RFR: 8241234: Unify monitor enter/exit runtime entries

[Daniel D. Daugherty]

Hi Yudi,

I grabbed a copy of this patch:

http://cr.openjdk.java.net/~yzheng/8241234/webrev.04/open.patch

pushed it into my jdk-15+16 baseline and ran it thru a single cycle of
my regular stress kit (~24 hours). There were no failures which matches
my jdk-15+16 baseline stress testing (~72 hours, no failures).

I also ran it through my ObjectMonitor inflation stress kit for ~24
hours and there were no failures there either.

Dan


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