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

[Patricio Chilano Mateo]

On Mon, 22 Mar 2021 20:16:38 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   add precondition assert() + suggested comments
>
> 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).

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

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