RFR: 8263896: Remove not_suspended parameter from ObjectMonitor::exit() [v2]

[Daniel D.Daugherty]

On Fri, 26 Mar 2021 17:07:56 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> 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.
>
> Hi Dan,
> 
>> 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.
> Thanks for the review. David would rather keep the use of the boolean here and just change it to have a default value of true, since that avoids using side state to decide whether we should set or not the previous owner. I prefer this approach of removing it altogether from the definition because I think this boolean is just a consequence of how we implement enter() rather than something the user of this class really cares about specifying when calling exit(). But in any case I don't mind using a parameter with default value since that also avoids having the callers of exit() worry about it. Do you also would rather keep not_suspended and set a default value of 'true' instead? If so, can I use this same PR to do that (because it says 'Remove' on the title).

@pchilano - I'm good with however you decide to solve this issue.
Normally, I hate default parameters, but this is a case where I can
see that having a default value of `true` makes sense.

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

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