8265753: Remove manual JavaThread transitions to blocked #3875

robehn · 2021-05-05T08:11:04Z

Please consider this change which removes the manual transitions to blocked.
This adds a preprocess template/functor which is executed in the destructor of 'ThreadBlockInVM' if we are going to do any processing.
This gives us a way to backout of the object/raw monitor before suspend or other processing, such as a safepoint.

The object monitor code could be straight forward changed to use this instead of manual transitions.

Raw monitors on the other hand are a bit more complicated due to 'implicit' rules (consequences of the specs).
Added a comment in hpp trying to explain it; we cannot simply transition with a raw monitor held.
This caused the change in the destructor ~ThreadInVMfromNative() (this specific change have also been tested in unrelated exploration of transition), now this RAII does the same as we do when going to native from Java, just setting the state.
Since we are going from an unsafe state, in VM, to a safe state, in native, we do not need to check the poll.
That made it possible to careful use ThreadInVMfromNative in raw monitors.

I also remove the early CAS in raw_enter.
We lock a lock to do a CAS, in the uncontended case means CAS on lock then CAS raw monitor.
Now we instead do a transitions, in the uncontended case means fence, CAS raw monitor, fence.
(multiple fence (CAS is also a fence) very close to each other have little additional performance impact on contemporary hardware)

Passes t1-t7 and manual stressing relevant test groups.

Progress

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

Issue

JDK-8265753: Remove manual JavaThread transitions to blocked

Reviewers

Daniel D. Daugherty (@dcubed-ojdk - Reviewer)
Richard Reingruber (@reinrich - Reviewer) ⚠️ Review applies to a8b8469
David Holmes (@dholmes-ora - Reviewer)
Patricio Chilano Mateo (@pchilano - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3875

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

Using diff file

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

bridgekeeper · 2021-05-05T08:12:22Z

👋 Welcome back rehn! 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 · 2021-05-05T08:14:11Z

@robehn The following labels will be automatically applied to this pull request:

hotspot
serviceability

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

robehn · 2021-05-05T08:18:46Z

/label remove hotspot
/label add hotspot-runtime

openjdk · 2021-05-05T08:19:13Z

@robehn
The hotspot label was successfully removed.

openjdk · 2021-05-05T08:20:21Z

@robehn
The hotspot-runtime label was successfully added.

mlbridge · 2021-05-05T11:54:06Z

Webrevs

dcubed-ojdk

Thumbs up on the over all logic. I only have minor nits and suggestions.

src/hotspot/share/prims/jvmtiRawMonitor.cpp

src/hotspot/share/prims/jvmtiRawMonitor.hpp

src/hotspot/share/runtime/interfaceSupport.inline.hpp

src/hotspot/share/runtime/objectMonitor.cpp

src/hotspot/share/runtime/objectMonitor.hpp

src/hotspot/share/runtime/objectMonitor.cpp

src/hotspot/share/runtime/interfaceSupport.inline.hpp

openjdk · 2021-05-11T19:09:39Z

@robehn 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:

8265753: Remove manual JavaThread transitions to blocked

Reviewed-by: dcubed, rrich, dholmes, pchilanomate

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 28 new commits pushed to the master branch:

de91643: 8267611: Print more info when pointer_delta assert fails
a4c46e1: 8263202: Update Hebrew/Indonesian/Yiddish ISO 639 language codes to current
9c346a1: 8266963: Remove safepoint poll introduced in 8262443 due to reentrance issue
45e0597: 8264302: Create implementation for Accessibility native peer for Splitpane java role
4343997: 8267708: Remove references to com.sun.tools.javadoc.**
f632254: 8267221: jshell feedback is incorrect when creating method with array varargs parameter
bf8d4a8: 8267583: jmod fails on symlink to class file
083416d: 8267130: Memory Overflow in Disassembler::load_library
9d305b9: 8252372: Check if cloning is required to move loads out of loops in PhaseIdealLoop::split_if_with_blocks_post()
0394416: 8267468: Rename refill waster counters in ThreadLocalAllocBuffer
... and 18 more: https://git.openjdk.java.net/jdk/compare/31d0f0d895ef4039d2e96a8fb6e990e93eed4d41...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.

robehn · 2021-05-12T08:06:02Z

Thumbs up on the over all logic. I only have minor nits and suggestions.

Thank you!

reinrich

Hi Robbin,

I haven't found the time for a proper review yet but I've experimented a little bit with lambdas. I could not make it work because g++ created references to ::new which isn't allowed.

Thanks, Richard.

src/hotspot/share/runtime/interfaceSupport.inline.hpp

robehn · 2021-05-12T10:53:21Z

Hi Robbin,

I haven't found the time for a proper review yet but I've experimented a little bit with lambdas. I could not make it work because g++ created references to ::new which isn't allowed.

Thanks, Richard.

Hi Richard,
I tested lamdba, which is just a fancy way to write a crazy typed functor, we need to capture the lamdba so we can run it in the destructor. AFAICT the way to do that is using std::function.

Regarding ThreadClosure, we could use it, maybe that is preferable?!

reinrich · 2021-05-12T12:31:42Z

Hi Robbin,
I haven't found the time for a proper review yet but I've experimented a little bit with lambdas. I could not make it work because g++ created references to ::new which isn't allowed.
Thanks, Richard.

Hi Richard,
I tested lamdba, which is just a fancy way to write a crazy typed functor, we need to capture the lamdba so we can run it in the destructor. AFAICT the way to do that is using std::function.

Yes, I'd think so too.

Regarding ThreadClosure, we could use it, maybe that is preferable?!

Personally I'd prefer it but I'm fine with the current version too.

dholmes-ora

Hi Robbin,

Sorry for the delay in getting through this.

Overall approach looks good. I have a few queries below and some requested naming changes to make things clearer.

Thanks,
David

src/hotspot/share/prims/jvmtiRawMonitor.hpp

src/hotspot/share/runtime/interfaceSupport.inline.hpp

src/hotspot/share/runtime/objectMonitor.cpp

src/hotspot/share/runtime/objectMonitor.hpp

reinrich

Hi Robbin,

there seem to be issues in the jvmtiRawMonitor part of the change. Besides that it looks good.

Cheers, Richard.

src/hotspot/share/prims/jvmtiRawMonitor.cpp

src/hotspot/share/prims/jvmtiRawMonitor.hpp

reinrich · 2021-05-14T17:53:15Z

src/hotspot/share/prims/jvmtiRawMonitor.cpp

+    _recursions = 0;
+    _waiters++;
+    ret = simple_wait(self, millis);
+    _waiters--;


We don't own the monitor yet so you cannot modify _waiters here.
Any reason you moved and duplicated this block?

Good catch! Even if the block wasn't moved we still no longer own the monitor after simple_wait.

This is going to be tricky to fix in a clear/clean way as the waiters count has to be maintained properly.

Actually the field is unused.
The _entry_list is the list of waiters, which is the only thing we really care about.
If there was a reader of _waiter reading it without lock can only be used as some kind of hint.
If you have lock you can just check _entry_list.

I duplicated it because otherwise I would need two branchse on if (self->is_Java_thread()) which creates more complicated code.

mlbridge · 2021-05-14T18:29:28Z

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

On 12/05/2021 8:56 pm, Robbin Ehn wrote:

On Wed, 12 May 2021 08:27:33 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

Hi Robbin,

I haven't found the time for a proper review yet but I've experimented a little bit with lambdas. I could not make it work because g++ created references to ::new which isn't allowed.

Thanks, Richard.

Hi Richard,
I tested lamdba, which is just a fancy way to write a crazy typed functor, we need to capture the lamdba so we can run it in the destructor. AFAICT the way to do that is using std::function.

Regarding ThreadClosure, we could use it, maybe that is preferable?!

Isn't a ThreadClosure for applying an operation to a set of threads?
That is not what we are doing here.

David

reinrich · 2021-05-14T19:24:06Z

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

On 12/05/2021 8:56 pm, Robbin Ehn wrote:

On Wed, 12 May 2021 08:27:33 GMT, Richard Reingruber wrote:

Hi Robbin,
I haven't found the time for a proper review yet but I've experimented a little bit with lambdas. I could not make it work because g++ created references to ::new which isn't allowed.
Thanks, Richard.

Hi Richard,
I tested lamdba, which is just a fancy way to write a crazy typed functor, we need to capture the lamdba so we can run it in the destructor. AFAICT the way to do that is using std::function.
Regarding ThreadClosure, we could use it, maybe that is preferable?!

Isn't a ThreadClosure for applying an operation to a set of threads?
That is not what we are doing here.

No it isn't. A closure is just a set of variable bindings and a function that can be executed. And yes we're doing just that.

A ThreadClosure is just an instance of this general concept. E.g. an AsyncHandshakeClosure (subclass of ThreadClosure) is a function (plus variable bindings) that is passed by a requesting thread to a target thread to be executed by it.

Thanks,
Richard.

mlbridge · 2021-05-17T07:25:12Z

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

On 17/05/2021 5:05 pm, Robbin Ehn wrote:

On Thu, 13 May 2021 05:22:51 GMT, David Holmes <dholmes at openjdk.org> wrote:

Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:

Fixes for Dan

src/hotspot/share/prims/jvmtiRawMonitor.hpp line 48:

46: // The rules are:
47: // - We must never safepoint poll if raw monitor is owned.
48: // - We may safepoint poll before it is owned and after it has been released.

I'm not sure exactly what this is trying to say because user code can acquire a RawMonitor, then call into Java while holding the RawMonitor. That external use of RawMonitors should never cause any deadlock with the VMThread of course.

This comment applies to the RawMonitor code, where the typical use-case that otherwise can deadlock is:
JavaThread:
-lock RM
LOOP {
-wait RM
-do stufff with data from VM thread
}
-unlock RM

The user do not call into the VM/Java.

VM Thread:
-safepoint
-lock RM
-notify RM
-unlock RM

If we in this case safepoint between the lock and the unlock in wait() we deadlock with VM thread.

If the user would call into the VM/Java while holding the RM he obviously could deadlock with VM thread.

Only if the VMThread executes code that uses the same RM - which should
be a rare occurrence.

David
-----

src/hotspot/share/prims/jvmtiRawMonitor.cpp

reinrich

Just one more, rather unimportant comment...

Either way: LGTM!

Thanks, Richard.

src/hotspot/share/prims/jvmtiRawMonitor.cpp

dholmes-ora

Updates seem fine. Lucky _waiters was unused :)

Will take another full pass over the changes.

Thanks,
David

src/hotspot/share/runtime/objectMonitor.hpp

dholmes-ora

Hi Robbin,

Overall this looks good to me, but there is one issue that needs fixing (partially pre-existing but now also affecting ObjectMonitor::enter).

Other minor comments below.

Thanks,
David

src/hotspot/share/runtime/interfaceSupport.inline.hpp

src/hotspot/share/runtime/objectMonitor.cpp

src/hotspot/share/prims/jvmtiRawMonitor.hpp

robehn · 2021-05-20T07:08:24Z

Just one more, rather unimportant comment...

Either way: LGTM!

Thanks, Richard.

Thanks!

mlbridge · 2021-05-20T07:21:03Z

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

On 20/05/2021 5:10 pm, Robbin Ehn wrote:

On Wed, 19 May 2021 14:25:41 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

Just one more, rather unimportant comment...

Either way: LGTM!

Thanks, Richard.

Thanks!
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382:

380:
381: _recursions = 0;
382: ret = simple_wait(self, millis);

IMHO the guarantee at L379 is redundant with the newly added identical guarantee in `JvmtiRawMonitor::simple_wait()` at L240.

In case you agree to remove the guarantee, I don't see why the following statements cannot be pulled out of the if-statement.
 \_recursions \= 0\;
 ret \= simple\_wait$self\, millis$\;
 \_recursions \= save\;
Yes, it can be re-written as something below, but is this more readable?

I'd say yes with two minor modifications:

_recursions = 0;
ret = simple_wait(self, millis);

+ // Now we need to re-enter the monitor. For JavaThread's
+ // we need to manage suspend requests.

if (self->is_Java_thread()) { // JavaThread re-enter
JavaThread* jt = self->as_Java_thread();
{

I think this extra block scope can also go.

Cheers,
David
-----

robehn · 2021-05-20T07:28:09Z

Hi Robbin,

Sorry for the delay in getting through this.

Overall approach looks good. I have a few queries below and some requested naming changes to make things clearer.

Thanks,
David

Thanks!

robehn · 2021-05-20T11:38:23Z

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

On 20/05/2021 5:10 pm, Robbin Ehn wrote:
On Wed, 19 May 2021 14:25:41 GMT, Richard Reingruber wrote:

Just one more, rather unimportant comment...
Either way: LGTM!
Thanks, Richard.

Thanks!
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382:

380:
381: _recursions = 0;
382: ret = simple_wait(self, millis);

IMHO the guarantee at L379 is redundant with the newly added identical guarantee in JvmtiRawMonitor::simple_wait() at L240.
In case you agree to remove the guarantee, I don't see why the following statements cannot be pulled out of the if-statement.
 \_recursions \= 0\;
 ret \= simple\_wait$self\, millis$\;
 \_recursions \= save\;
Yes, it can be re-written as something below, but is this more readable?
I'd say yes with two minor modifications:

_recursions = 0;
ret = simple_wait(self, millis);

// Now we need to re-enter the monitor. For JavaThread's

// we need to manage suspend requests.

if (self->is_Java_thread()) { // JavaThread re-enter
JavaThread* jt = self->as_Java_thread();
{

I think this extra block scope can also go.

Cheers,
David

Fixed, thanks, Robbin

dholmes-ora

Updates mostly good but still a storestore issue to resolve.

Thanks,
David

src/hotspot/share/prims/jvmtiRawMonitor.cpp

src/hotspot/share/runtime/interfaceSupport.inline.hpp

robehn · 2021-05-24T06:43:06Z

Are we good to go?

Thanks, Robbin

dcubed-ojdk

Thumbs up.

I just have some minor questions about a couple of the details.
Nothing blocking.

src/hotspot/share/prims/jvmtiRawMonitor.cpp

src/hotspot/share/runtime/interfaceSupport.inline.hpp

src/hotspot/share/runtime/objectMonitor.cpp

dcubed-ojdk · 2021-05-24T17:57:44Z

src/hotspot/share/runtime/objectMonitor.cpp

    // We cleared the pending monitor info since we've just gotten past
    // the enter-check-for-suspend dance and we now own the monitor free
    // and clear, i.e., it is no longer pending.


This comment explains why the set-pending-to-NULL code was here
(outside the loop). You moved the code, but now the comment is
misplaced and the code no longer works the same.

Update: I found this resolved comment below:

And I set current->set_current_pending_monitor(om); to OM again in ExitOnSuspend if we exit the OM.

so that explains why we don't have a big change in behavior.

However, I think you still need to do something about this comment.

There is still an observable change in behavior in that the current
pending monitor can cycle between set->NULL->set->NULL when
we run into a suspend request. Previously, it did not cycle.

If we stop for a suspend request current pending monitor will always be set, so the cycling cannot be seen.
Updated comment(s).

robehn · 2021-05-25T07:09:20Z

Thumbs up.

I just have some minor questions about a couple of the details.
Nothing blocking.

Thanks Dan!

reinrich · 2021-05-25T13:04:46Z

Still looks good to me.

Cheer's, Richard.

robehn · 2021-05-25T13:35:19Z

Still looks good to me.

Cheer's, Richard.

Thank you!

dholmes-ora

Nothing further from me.

Thanks,
David

robehn · 2021-05-26T06:21:38Z

Nothing further from me.

Thanks,
David

Thank you David!

dcubed-ojdk

Thumbs up.

Reviewed the incremental between v05 -> v06. Looks good.

src/hotspot/share/runtime/interfaceSupport.inline.hpp

robehn · 2021-05-28T07:27:39Z

Thumbs up.

Reviewed the incremental between v05 -> v06. Looks good.

Thanks!

robehn · 2021-05-28T07:29:21Z

/integrate

openjdk · 2021-05-28T07:30:39Z

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

6eb9114: 8266877: Missing local debug information when debugging JEP-330
0c9daa7: 8265029: Preserve SIZED characteristics on slice operations (skip, limit)
95b1fa7: 8267529: StringJoiner can create a String that breaks String::equals
7f52c50: 8182043: Access to Windows Large Icons
8a31c07: 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java
ae258f1: 8265418: Clean-up redundant null-checks of Class.getPackageName()
41185d3: 8229517: Support for optional asynchronous/buffered logging
7c85f35: 8267123: Remove RMI Activation
0754266: 8267709: Investigate differences between HtmlStyle and stylesheet.css
23189a1: 8191786: Thread-SMR hash table size should be dynamic
... and 44 more: https://git.openjdk.java.net/jdk/compare/31d0f0d895ef4039d2e96a8fb6e990e93eed4d41...master

Your commit was automatically rebased without conflicts.

Pushed as commit 97ec5ad.

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

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels May 5, 2021

openjdk bot removed the hotspot hotspot-dev@openjdk.org label May 5, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label May 5, 2021

Removed manual transitions

e16e2c3

robehn force-pushed the 8265753 branch from a5fe75d to e16e2c3 Compare May 5, 2021 08:46

robehn marked this pull request as ready for review May 5, 2021 11:49

openjdk bot added the rfr Pull request is ready for review label May 5, 2021

Merge branch 'master' into 8265753

07ddf39

dcubed-ojdk approved these changes May 11, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 11, 2021

Fixes for Dan

1c6448e

reinrich reviewed May 12, 2021

View reviewed changes

src/hotspot/share/runtime/interfaceSupport.inline.hpp Show resolved Hide resolved

dholmes-ora suggested changes May 13, 2021

View reviewed changes

reinrich suggested changes May 14, 2021

View reviewed changes

reinrich reviewed May 17, 2021

View reviewed changes

src/hotspot/share/prims/jvmtiRawMonitor.cpp Outdated Show resolved Hide resolved

Merge branch 'master' into 8265753

7713f3e

reinrich approved these changes May 19, 2021

View reviewed changes

src/hotspot/share/prims/jvmtiRawMonitor.cpp Outdated Show resolved Hide resolved

dholmes-ora reviewed May 20, 2021

View reviewed changes

src/hotspot/share/runtime/objectMonitor.hpp Outdated Show resolved Hide resolved

Merge branch 'master' into 8265753

3e1c212

dholmes-ora suggested changes May 20, 2021

View reviewed changes

Review fixes 2

6faf142

dholmes-ora suggested changes May 20, 2021

View reviewed changes

src/hotspot/share/prims/jvmtiRawMonitor.cpp Outdated Show resolved Hide resolved

src/hotspot/share/runtime/interfaceSupport.inline.hpp Outdated Show resolved Hide resolved

Final fixes: last famous words

cc15431

dcubed-ojdk approved these changes May 24, 2021

View reviewed changes

robehn added 2 commits May 25, 2021 08:37

Merge branch 'master' into 8265753

2c4e71b

Fixes for Dan

30d62e8

dholmes-ora approved these changes May 25, 2021

View reviewed changes

dcubed-ojdk approved these changes May 26, 2021

View reviewed changes

pchilano approved these changes May 26, 2021

View reviewed changes

src/hotspot/share/runtime/interfaceSupport.inline.hpp Show resolved Hide resolved

openjdk bot closed this May 28, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels May 28, 2021

openjdk bot removed the rfr Pull request is ready for review label May 28, 2021

robehn deleted the 8265753 branch May 28, 2021 07:34

8265753: Remove manual JavaThread transitions to blocked #3875

8265753: Remove manual JavaThread transitions to blocked #3875

Uh oh!

Conversation

robehn commented May 5, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 5, 2021

Uh oh!

openjdk bot commented May 5, 2021

Uh oh!

robehn commented May 5, 2021

Uh oh!

openjdk bot commented May 5, 2021

Uh oh!

openjdk bot commented May 5, 2021

Uh oh!

mlbridge bot commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dcubed-ojdk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robehn commented May 12, 2021

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robehn commented May 12, 2021

Uh oh!

reinrich commented May 12, 2021

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reinrich May 14, 2021

Choose a reason for hiding this comment

Uh oh!

dholmes-ora May 17, 2021

Choose a reason for hiding this comment

Uh oh!

robehn May 17, 2021

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented May 14, 2021

Uh oh!

robehn commented May 5, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented May 5, 2021 •

edited

Loading

dcubed-ojdk left a comment •

edited

Loading

openjdk bot commented May 11, 2021 •

edited

Loading

Cheers,
David