RFR: 8263896: Remove not_suspended parameter from ObjectMonitor::exit()

[Daniel D.Daugherty]

On Sat, 20 Mar 2021 00:24:43 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Hi,
> 
> Please review the following small patch. The boolean parameter not_suspended is used to detect if we need to set the current JavaThread exiting the monitor as the previous owner (_previous_owner_tid). If not_suspended is true then we set _previous_owner_tid, otherwise we skip the write (modulo the JFR checks). This parameter is always true except when we call exit() from inside enter(). This happens when the JavaThread acquires the monitor but notices that it was suspended while being in the _thread_blocked state. Since in that case the JT was never really "the owner" we skip setting _previous_owner_tid.
> 
> This behaviour of releasing the monitor is just an implementation detail of ObjectMonitor::enter() which doesn't need to be exposed in the exit() API. We can identify the same scenario just by checking _current_pending_monitor instead.
> 
> Thanks,
> Patricio

Thumbs up.

src/hotspot/share/runtime/objectMonitor.cpp line 1182:

> 1180: #if INCLUDE_JFR
> 1181:   // set _previous_owner_tid for the MonitorEnter event if it is enabled and
> 1182:   // the thread isn't releasing the monitor from inside enter()

nit - need a period and the end of the comment.

src/hotspot/share/runtime/objectMonitor.cpp line 1185:

> 1183:   if (current->current_pending_monitor() == NULL && EventJavaMonitorEnter::is_enabled()) {
> 1184:     _previous_owner_tid = JFR_THREAD_ID(current);
> 1185:   }

Instead of gating this piece of code on a flag that's passed in from
the code in enter() that calls exit() under the special case, you're
switching to the `current_pending_monitor` field which is set and
cleared by the code in enter(). It's only set when there is a contended
enter and only cleared when we've successfully entered after a
contended enter. Entering while there's a pending suspend request is
not counted as a successful enter which is why the original code has
that flag to exit().

I think this change makes the linkage between the semantics of
enter() and exit() more brittle and I'm not fond of this. However,
I agree that this change makes the linkage between enter() and
exit() an implementation detail instead of exposed by the API. Of
course, only someone that is intimately familiar with the
implementation is going to even grok the reasons for that 3-line
block of code.

Okay. Originally I was going to object, but I've decided that making
this an implementation detail is better.

-------------

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3101