Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

@pchilano
Copy link
Contributor

@pchilano pchilano commented Mar 20, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

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

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3101/head:pull/3101
$ git checkout pull/3101

Update a local copy of the PR:
$ git checkout pull/3101
$ git pull https://git.openjdk.java.net/jdk pull/3101/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3101

View PR using the GUI difftool:
$ git pr show -t 3101

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3101.diff

v1
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 20, 2021

👋 Welcome back pchilanomate! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 20, 2021

@pchilano The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@pchilano pchilano marked this pull request as ready for review Mar 22, 2021
@openjdk openjdk bot added the rfr label Mar 22, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 22, 2021

Webrevs

@robehn
robehn approved these changes Mar 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 22, 2021

@pchilano This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: rehn, dcubed, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 155 new commits pushed to the master branch:

  • ee5e00b: 8264279: Shenandoah: Missing handshake after JDK-8263427
  • ac604a1: 8264374: Shenandoah: Remove leftover parallel reference processing argument
  • f3726a8: 8264020: Optimize double negation elimination
  • 4ffa41c: 8263615: Cleanup tightly_coupled_allocation
  • 4ea6abf: 8264324: Simplify allocation list management in OopStorage::reduce_deferred_updates
  • 8735259: 8264333: Use the blessed modifier order in jdk.jshell
  • d2a63f2: 8264360: Loop strip mining verification fails with "should be on the backedge"
  • 8100a20: 8263971: C2 crashes with SIGFPE with -XX:+StressGCM and -XX:+StressIGVN
  • bcdf469: 8264337: VM crashed when -XX:+VerifySharedSpaces
  • 1a681fa: 8263560: Remove needless wrapping with BufferedInputStream
  • ... and 145 more: https://git.openjdk.java.net/jdk/compare/96e5c3f1e0c0cfc9bbc77a357bdf3d169100c07e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Mar 22, 2021
Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up.

// if it is enabled and the thread isn't suspended
if (not_suspended && EventJavaMonitorEnter::is_enabled()) {
// set _previous_owner_tid for the MonitorEnter event if it is enabled and
// the thread isn't releasing the monitor from inside enter()

This comment has been minimized.

@dcubed-ojdk

dcubed-ojdk Mar 22, 2021
Member

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

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 23, 2021
Member

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.

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

This comment has been minimized.

@dcubed-ojdk

dcubed-ojdk Mar 22, 2021
Member

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.

This comment has been minimized.

@pchilano

pchilano Mar 26, 2021
Author Contributor

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

This comment has been minimized.

@dcubed-ojdk

dcubed-ojdk Mar 26, 2021
Member

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

This comment has been minimized.

@pchilano

pchilano Mar 26, 2021
Author Contributor

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

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Mar 22, 2021

@dholmes-ora may have an opinion here.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

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

// if it is enabled and the thread isn't suspended
if (not_suspended && EventJavaMonitorEnter::is_enabled()) {
// set _previous_owner_tid for the MonitorEnter event if it is enabled and
// the thread isn't releasing the monitor from inside enter()

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 23, 2021
Member

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 23, 2021

Mailing list message from patricio.chilano.mateo at oracle.com on hotspot-runtime-dev:

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 23, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 23/03/2021 4:11 pm, patricio.chilano.mateo at oracle.com 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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2021

Mailing list message from patricio.chilano.mateo at oracle.com on hotspot-runtime-dev:

Hi David,

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

On 23/03/2021 4:11 pm, patricio.chilano.mateo at oracle.com 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

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

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.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

This looks good to me - thanks.

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

Thanks,
David

@@ -388,6 +388,7 @@ bool ObjectMonitor::enter(JavaThread* current) {
{ // Change java thread status to indicate blocked on monitor enter.
JavaThreadBlockedOnMonitorEnterState jtbmes(current, this);

assert(current->current_pending_monitor() == NULL, "invariant");

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 29, 2021
Member

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.

@robehn
robehn approved these changes Mar 29, 2021
@pchilano pchilano changed the title 8263896: Remove not_suspended parameter from ObjectMonitor::exit() 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value Mar 29, 2021
@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Mar 29, 2021

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

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Mar 30, 2021

/integrate

@openjdk openjdk bot closed this Mar 30, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

@pchilano Since your change was applied there have been 160 commits pushed to the master branch:

  • b652198: 8264429: Test runtime/cds/appcds/VerifyWithDefaultArchive.java assumes OpenJDK build
  • 2c9365d: 8264271: Avoid creating non_oop_word oops
  • daeca3f: 8262958: (fs) UnixUserDefinedFileAttributeView cleanup
  • af02883: 8264191: Javadoc search is broken in Internet Explorer
  • 6e74c3a: 8264193: Remove TRAPS parameters for modules and defaultmethods
  • ee5e00b: 8264279: Shenandoah: Missing handshake after JDK-8263427
  • ac604a1: 8264374: Shenandoah: Remove leftover parallel reference processing argument
  • f3726a8: 8264020: Optimize double negation elimination
  • 4ffa41c: 8263615: Cleanup tightly_coupled_allocation
  • 4ea6abf: 8264324: Simplify allocation list management in OopStorage::reduce_deferred_updates
  • ... and 150 more: https://git.openjdk.java.net/jdk/compare/96e5c3f1e0c0cfc9bbc77a357bdf3d169100c07e...master

Your commit was automatically rebased without conflicts.

Pushed as commit 2ad6f2d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants