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

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Patricio Chilano Mateo
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

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

Commit messages:
 - v1

Changes: https://git.openjdk.java.net/jdk/pull/3101/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3101&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263896
  Stats: 10 lines in 3 files changed: 0 ins; 0 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3101.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3101/head:pull/3101

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

Robbin Ehn-2
On Sat, 20 Mar 2021 00:24:43 GMT, Patricio Chilano Mateo <[hidden email]> 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

Marked as reviewed by rehn (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

Daniel D.Daugherty
In reply to this post by Patricio Chilano Mateo
On Sat, 20 Mar 2021 00:24:43 GMT, Patricio Chilano Mateo <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Daniel D.Daugherty
On Mon, 22 Mar 2021 20:17:00 GMT, Daniel D. Daugherty <[hidden email]> 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.

@dholmes-ora may have an opinion here.

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

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

David Holmes-2
In reply to this post by Patricio Chilano Mateo
On Sat, 20 Mar 2021 00:24:43 GMT, Patricio Chilano Mateo <[hidden email]> 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

Hi Patricio,

I'm in two-minds about using incidental state to infer whether to store the last_owner_tid here. Someone might reasonably argue that we should actually be clearing the pending-monitor field whilst in the self-suspend loop.

Is this going to feed into Robbin's thread suspension changes? Otherwise I'm not really seeing the motivation.

Thanks,
David

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

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

David Holmes-2
In reply to this post by Daniel D.Daugherty
On Mon, 22 Mar 2021 20:14:49 GMT, Daniel D. Daugherty <[hidden email]> 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
>
> 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.

This is rather a bit too obscure so I think the comment needs to be expanded.
// Set _previous_owner_tid for the MonitorEnter event if it is enabled and
// we legitimately owned this monitor. We can also get here if we need to self-suspend
// in enter(), in which case we never really owned this monitor and so should not record
// our thread id. In that case current_pending_monitor() is non-NULL.

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

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

Patricio Chilano
In reply to this post by David Holmes-2
Hi David,

On 3/23/21 12:08 AM, David Holmes wrote:
> Hi Patricio,
>
> I'm in two-minds about using incidental state to infer whether to store the last_owner_tid here. Someone might reasonably argue that we should actually be clearing the pending-monitor field whilst in the self-suspend loop.
Right, but having _current_pending_monitor != NULL when calling exit()
can only happen if we are releasing the monitor from enter(), which we
only do in case we were suspended, so both things go hand in hand. If we
want to clear _current_pending_monitor while being suspended we can do
it after the exit() call though and before calling java_suspend_self().

> Is this going to feed into Robbin's thread suspension changes? Otherwise I'm not really seeing the motivation.
Not really. It's just to avoid having to declare exit() with this extra
parameter that's only used for a special case that we can already
detect. It seems wrong to have the user calling exit() figure out what
this value should be.
Alternatively since your cleanup in 8262910, I could move it to the end
of the parameter list and set a default value of true. Then only pass it
explicitly as false for this call. (although I would prefer removing it
altogether and just use _current_pending_monitor : ) ).

Thanks,
Patricio
> Thanks,
> David
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3101

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 23/03/2021 4:11 pm, [hidden email] wrote:

> Hi David,
>
> On 3/23/21 12:08 AM, David Holmes wrote:
>> Hi Patricio,
>>
>> I'm in two-minds about using incidental state to infer whether to
>> store the last_owner_tid here. Someone might reasonably argue that we
>> should actually be clearing the pending-monitor field whilst in the
>> self-suspend loop.
> Right, but having _current_pending_monitor != NULL when calling exit()
> can only happen if we are releasing the monitor from enter(), which we
> only do in case we were suspended, so both things go hand in hand.

But they only happen to go hand-in-hand due to the way the code is
currently structured. If I'm not mistaken you could also check that the
thread state is _thread_blocked, or the osThread has "contend" state, as
both of these will also only be true when the exit() call comes from
within enter(). These are all things that happen to be true, but which
someone could inadvertently change not realising the code structure was
being used to indicate something to another piece of code.

>If we
> want to clear _current_pending_monitor while being suspended we can do
> it after the exit() call though and before calling java_suspend_self().

Yes but you have to know that it must be done after the exit() call
rather than say:

       current->set_current_pending_monitor(this);
       EnterI(current);
       current->set_current_pending_monitor(NULL);

>> Is this going to feed into Robbin's thread suspension changes?
>> Otherwise I'm not really seeing the motivation.
> Not really. It's just to avoid having to declare exit() with this extra
> parameter that's only used for a special case that we can already
> detect. It seems wrong to have the user calling exit() figure out what
> this value should be.
> Alternatively since your cleanup in 8262910, I could move it to the end
> of the parameter list and set a default value of true. Then only pass it
> explicitly as false for this call. (although I would prefer removing it
> altogether and just use _current_pending_monitor : ) ).

Yes a default parameter would be clearer. Personally I don't mind
default parameters for these kind of special cases. And as you note
previously this couldn't be a default parameter but now it can.

So my vote would be for a default parameter, but if everyone prefers
this then so be it. :)

Thanks,
David
-----

>
> Thanks,
> Patricio
>> Thanks,
>> David
>>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3101
>
Reply | Threaded
Open this post in threaded view
|

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

Patricio Chilano
Hi David,

On 3/23/21 4:18 AM, David Holmes wrote:

> On 23/03/2021 4:11 pm, [hidden email] wrote:
>> Hi David,
>>
>> On 3/23/21 12:08 AM, David Holmes wrote:
>>> Hi Patricio,
>>>
>>> I'm in two-minds about using incidental state to infer whether to
>>> store the last_owner_tid here. Someone might reasonably argue that
>>> we should actually be clearing the pending-monitor field whilst in
>>> the self-suspend loop.
>> Right, but having _current_pending_monitor != NULL when calling
>> exit() can only happen if we are releasing the monitor from enter(),
>> which we only do in case we were suspended, so both things go hand in
>> hand.
>
> But they only happen to go hand-in-hand due to the way the code is
> currently structured. If I'm not mistaken you could also check that
> the thread state is _thread_blocked, or the osThread has "contend"
> state, as both of these will also only be true when the exit() call
> comes from within enter(). These are all things that happen to be
> true, but which someone could inadvertently change not realising the
> code structure was being used to indicate something to another piece
> of code.
>
>> If we want to clear _current_pending_monitor while being suspended we
>> can do it after the exit() call though and before calling
>> java_suspend_self().
>
> Yes but you have to know that it must be done after the exit() call
> rather than say:
>
>       current->set_current_pending_monitor(this);
>       EnterI(current);
>       current->set_current_pending_monitor(NULL);
Ok, I can add an assert right before that special exit() call to check
that _current_pending_monitor is set. That will catch code refactoring
or other changes since the exit() call and the precondition check will
be right next to each other.

>>> Is this going to feed into Robbin's thread suspension changes?
>>> Otherwise I'm not really seeing the motivation.
>> Not really. It's just to avoid having to declare exit() with this
>> extra parameter that's only used for a special case that we can
>> already detect. It seems wrong to have the user calling exit() figure
>> out what this value should be.
>> Alternatively since your cleanup in 8262910, I could move it to the
>> end of the parameter list and set a default value of true. Then only
>> pass it explicitly as false for this call. (although I would prefer
>> removing it altogether and just use _current_pending_monitor : ) ).
>
> Yes a default parameter would be clearer. Personally I don't mind
> default parameters for these kind of special cases. And as you note
> previously this couldn't be a default parameter but now it can.
>
> So my vote would be for a default parameter, but if everyone prefers
> this then so be it. :)
Ok, I updated the pull request adding that assert() and the comments but
if you still prefer the default parameter way I can do that.

Thanks David!

Patricio

> Thanks,
> David
> -----
>
>>
>> Thanks,
>> Patricio
>>> Thanks,
>>> David
>>>
>>> -------------
>>>
>>> PR: https://git.openjdk.java.net/jdk/pull/3101
>>

Reply | Threaded
Open this post in threaded view
|

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

Patricio Chilano Mateo
In reply to this post by Patricio Chilano Mateo
> 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

Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:

  add precondition assert() + suggested comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3101/files
  - new: https://git.openjdk.java.net/jdk/pull/3101/files/5eed17cc..629a17aa

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3101&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3101&range=00-01

  Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3101.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3101/head:pull/3101

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

Patricio Chilano Mateo
In reply to this post by Daniel D.Daugherty
On Mon, 22 Mar 2021 20:16:38 GMT, Daniel D. Daugherty <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Patricio Chilano Mateo
In reply to this post by Patricio Chilano Mateo
> 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

Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:

  restore not_suspended with default argument

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3101/files
  - new: https://git.openjdk.java.net/jdk/pull/3101/files/629a17aa..c5d604c6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3101&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3101&range=01-02

  Stats: 11 lines in 2 files changed: 0 ins; 5 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3101.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3101/head:pull/3101

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

Patricio Chilano Mateo
In reply to this post by Daniel D.Daugherty
On Fri, 26 Mar 2021 19:17:43 GMT, Daniel D. Daugherty <[hidden email]> wrote:

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

Ok, thanks Dan. Please review the update. I still kept the assert() on current_pending_monitor. Should I modify the title of the PR?

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

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

Daniel D.Daugherty
In reply to this post by Patricio Chilano Mateo
On Fri, 26 Mar 2021 20:34:43 GMT, Patricio Chilano Mateo <[hidden email]> 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
>
> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>
>   restore not_suspended with default argument

Thumbs up!

I don't care if you leave the PR title as is. If you change it, then you'll
have to change the bug synopsis also or Skara won't like it.

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

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

David Holmes-2
In reply to this post by Patricio Chilano Mateo
On Fri, 26 Mar 2021 20:34:43 GMT, Patricio Chilano Mateo <[hidden email]> 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
>
> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>
>   restore not_suspended with default argument

This looks good to me - thanks.

Please update the bug and the PR title to reflect that the parameter was not removed.

Thanks,
David

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

> 389:     JavaThreadBlockedOnMonitorEnterState jtbmes(current, this);
> 390:
> 391:     assert(current->current_pending_monitor() == NULL, "invariant");

Really this belongs inside set_current_pending_monitor() where the only allowed transitions are null to non-null, and non-null to null. But that would be a separate RFE.

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

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

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

Robbin Ehn-2
In reply to this post by Patricio Chilano Mateo
On Fri, 26 Mar 2021 20:34:43 GMT, Patricio Chilano Mateo <[hidden email]> 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
>
> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>
>   restore not_suspended with default argument

Marked as reviewed by rehn (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value [v3]

Patricio Chilano Mateo
On Mon, 29 Mar 2021 06:50:36 GMT, Robbin Ehn <[hidden email]> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   restore not_suspended with default argument
>
> Marked as reviewed by rehn (Reviewer).

Thanks @robehn, @dcubed-ojdk and @dholmes-ora for the reviews!

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

PR: https://git.openjdk.java.net/jdk/pull/3101
Reply | Threaded
Open this post in threaded view
|

Integrated: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value

Patricio Chilano Mateo
In reply to this post by Patricio Chilano Mateo
On Sat, 20 Mar 2021 00:24:43 GMT, Patricio Chilano Mateo <[hidden email]> 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

This pull request has now been integrated.

Changeset: 2ad6f2d9
Author:    Patricio Chilano Mateo <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/2ad6f2d9
Stats:     8 lines in 3 files changed: 1 ins; 0 del; 7 mod

8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value

Reviewed-by: rehn, dcubed, dholmes

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

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