RFR: 8241234: Unify monitor enter/exit runtime entries
yudi.zheng at oracle.com
Wed Apr 1 20:23:02 UTC 2020
Thanks a lot for stress testing this patch!
I will push this as soon as I get green lights from the mach5 tests.
> On 1 Apr 2020, at 20:45, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
> Hi Yudi,
> I grabbed a copy of this 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.
> 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:
>>>> Please don't forget to update the copyright year before you push.
>>>> 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.
>>>> 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.
>>>> 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.
>>> Many thanks,
More information about the hotspot-runtime-dev