8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers #2880

pchilano · 2021-03-08T19:33:06Z

Hi,

Please review the following small patch. ThreadInVMfromJavaNoAsyncException is exactly the same as ThreadInVMfromJava except that in the destructor we avoid checking for asynchronous exceptions. Similarly ThreadBlockInVMWithDeadlockCheck is equal to ThreadBlockInVM except that in the destructor we might need to release a Mutex in case we need to stop for a safepoint or handshake.
We can consolidate these two wrappers with the existing ones to avoid code duplication and minimize the number of transition wrappers.

Tested in mach5 tiers 1-3.

Thanks,
Patricio

Progress

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

Issue

JDK-8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers

Reviewers

Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to e99ca59
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to e99ca59
Daniel D. Daugherty (@dcubed-ojdk - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2880/head:pull/2880
$ git checkout pull/2880

bridgekeeper · 2021-03-08T19:33:14Z

👋 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 · 2021-03-08T19:34:05Z

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

mlbridge · 2021-03-08T20:56:13Z

Webrevs

01: Full - Incremental (865f2d6)
00: Full (e99ca59)

coleenp

Looks good!

openjdk · 2021-03-09T01:39:25Z

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

8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers

Reviewed-by: coleenp, dholmes, dcubed

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

f3bd801: 8263403: [JVMCI] output written to tty via HotSpotJVMCIRuntime can be garbled
b92abac: 8263433: Shenandoah: Don't expect forwarded objects in set_concurrent_mark_in_progress()
15dacca: 8263465: JDK-8236847 causes tier1 build failure on linux-aarch64
7ed46bd: 8241716: Jpackage functionality to let users choose whether to create shortcuts
3820ab9: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages
273f8bd: 8263248: IGV: accept graphs without node categories
a9b4f03: 8263069: Exclude some failing tests from security/infra/java/security/cert/CertPathValidator
470b150: 8143041: Unify G1CollectorPolicy::PauseKind and G1YCType
f6b4ba0: 8261931: IGV: quick search fails on multi-line node labels
7988c1d: 8262443: GenerateOopMap::do_interpretation can spin for a long time.
... and 46 more: https://git.openjdk.java.net/jdk/compare/679faa691adba139c698ec2e34f71f452b6400ad...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.

dholmes-ora

Looks good Patricio!

Thanks,
David

dcubed-ojdk

I think there are non-trivial differences between the old ThreadBlockInVM
and the new ThreadBlockInVM. Maybe I'm wrong, but they do not look
equivalent to me.

dcubed-ojdk · 2021-03-09T15:17:07Z

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

 public:
-  ThreadInVMfromJava(JavaThread* thread) : ThreadStateTransition(thread) {
+  ThreadInVMfromJava(JavaThread* thread, bool check_async = true) : ThreadStateTransition(thread), _check_asyncs(check_async) {


Field is _check_asyncs and parameter is check_async.
Either add an s to the parameter or delete the s from the field.

dcubed-ojdk · 2021-03-09T15:37:34Z

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

 public:
-  ThreadBlockInVMWithDeadlockCheck(JavaThread* thread, Mutex** in_flight_mutex_addr)
+  ThreadBlockInVM(JavaThread* thread, Mutex** in_flight_mutex_addr = NULL)


The new ThreadBlockInVM is not equivalent to the old one.
The old one used trans(_thread_in_vm, _thread_blocked) which
asserted that the "from" state is equal to _thread_in_vm. This
new code does not do that. The trans() call also resulted in a
SafepointMechanism::process_if_requested(thread); call after
we transitioned the thread to _thread_in_vm+1 and before we
set the new thread state to _thread_blocked.

dcubed-ojdk · 2021-03-09T15:42:26Z

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

-    // All unsafe states are treated the same by the VMThread
-    // so we can skip the _thread_in_vm_trans state here. Since
-    // we don't read poll, it's enough to order the stores.
-    OrderAccess::storestore();
-
    thread->set_thread_state(_thread_blocked);


If you're going to set the thread_state directly here, I think you need
to keep the OrderAccess::storestore() call. However, this new ThreadBlockInVM
is also missing a SafepointMechanism::process_if_requested(thread) call that
happened before setting the _thread_blocked state.

mlbridge · 2021-03-09T20:34:08Z

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

Hi Dan,

Thanks for looking at this.

On 3/9/21 12:53 PM, Daniel D.Daugherty wrote:

I think there are non-trivial differences between the old ThreadBlockInVM
and the new ThreadBlockInVM. Maybe I'm wrong, but they do not look
equivalent to me.

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 155:

153: bool _check_asyncs;
154: public:
155: ThreadInVMfromJava(JavaThread* thread, bool check_async = true) : ThreadStateTransition(thread), _check_asyncs(check_async) {
Field is `_check_asyncs` and parameter is `check_async`.
Either add an `s` to the parameter or delete the `s` from the field.

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 237:

Fixed.

235:
236: public:
237: ThreadBlockInVM(JavaThread* thread, Mutex** in_flight_mutex_addr = NULL)
The new ThreadBlockInVM is not equivalent to the old one.
The old one used `trans(_thread_in_vm, _thread_blocked)` which
asserted that the "from" state is equal to `_thread_in_vm`. This
new code does not do that.

Fixed. I added an assert() that the thread state is _thread_in_vm in
TBIVM() and _thread_blocked in ~TBIVM().

The trans() call also resulted in a
`SafepointMechanism::process_if_requested(thread);` call after
we transitioned the thread to `_thread_in_vm`+1 and before we
set the new thread state to `_thread_blocked`.

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 242:

240: // Once we are blocked vm expects stack to be walkable
241: thread->frame_anchor()->make_walkable(thread);
242: thread->set_thread_state(_thread_blocked);
If you're going to set the thread_state directly here, I think you need
to keep the `OrderAccess::storestore()` call. However, this new ThreadBlockInVM
is also missing a `SafepointMechanism::process_if_requested(thread)` call that
happened before setting the `_thread_blocked` state.

Calling explicitly SafepointMechanism::process_if_requested() in TBIVM()
is not needed because we are setting the state to blocked. That will
already allow any safepoint/handshake to proceed. If when reaching
~TBIVM() the poll is armed then we will go through
process_if_requested() there.
The reason I removed OrderAccess::storestore() is because
thread->set_thread_state() is a release store already.

Thanks,
Patricio

dcubed-ojdk · 2021-03-11T20:52:16Z

I think you're missing my point about the equivalence of the old ThreadBlockInVM()
and the new ThreadBlockInVM(). The old version did this in trans()/transition():

    // Change to transition state and ensure it is seen by the VM thread.
    thread->set_thread_state_fence((JavaThreadState)(from + 1));

    SafepointMechanism::process_if_requested(thread);
    thread->set_thread_state(to);

which means that when we were in (_thread_blocked+1) we did a
process_if_requested(thread) call before we changed the thread's
state to _thread_blocked. In the new code, we just got straight to
_thread_blocked without processing any pending request. I think the
only case that matters is that we won't process a pending handshake
request. As you say, a pending safepoint will be allowed to proceed
once we've reached _thread_blocked.

It could be that this change in behavior doesn't make any real difference,
but I want to be sure.

dcubed-ojdk

Thanks for making the fixes you've done so far, but I still
have one nagging question...

dcubed-ojdk

Patricio and I have bounced this change in behavior back and forth
in Slack. I'm now convinced that it is a benign change in behavior.

mlbridge · 2021-03-11T21:39:08Z

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

On 3/11/21 6:10 PM, Daniel D.Daugherty wrote:

On Tue, 9 Mar 2021 20:32:18 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

Hi,

Please review the following small patch. ThreadInVMfromJavaNoAsyncException is exactly the same as ThreadInVMfromJava except that in the destructor we avoid checking for asynchronous exceptions. Similarly ThreadBlockInVMWithDeadlockCheck is equal to ThreadBlockInVM except that in the destructor we might need to release a Mutex in case we need to stop for a safepoint or handshake.
We can consolidate these two wrappers with the existing ones to avoid code duplication and minimize the number of transition wrappers.

Tested in mach5 tiers 1-3.

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

add asserts for thread state + check_asyncs name
Patricio and I have bounced this change in behavior back and forth
in Slack. I'm now convinced that it is a benign change in behavior.

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

Great, thanks Dan!
Just for the record on the previous concerns below..

I think there are non-trivial differences between the old ThreadBlockInVM
and the new ThreadBlockInVM. Maybe I'm wrong, but they do not look
equivalent to me.
I think you're missing my point about the equivalence of the old ThreadBlockInVM()
and the new ThreadBlockInVM(). The old version did this in trans()/transition():
 \/\/ Change to transition state and ensure it is seen by the VM thread\.
 thread\->set\_thread\_state\_fence$\(JavaThreadState$$from \+ 1$\)\;

 SafepointMechanism\:\:process\_if\_requested$thread$\;
 thread\->set\_thread\_state$to$\;
which means that when we were in (`_thread_blocked`+1) we did a
`process_if_requested(thread)` call before we changed the thread's
state to `_thread_blocked`. In the new code, we just got straight to
`_thread_blocked` without processing any pending request. I think the
only case that matters is that we won't process a pending handshake
request. As you say, a pending safepoint will be allowed to proceed
once we've reached `_thread_blocked`.

It could be that this change in behavior doesn't make any real difference,
but I want to be sure.

When the JT changes its state to blocked it will also be handshake safe,
so both safepoints and handshakes will be able to continue. Also polling
in the constructor doesn't really change anything because it could be
that the poll gets armed right after calling
SafepointMechanism::process_if_requested(), so we should still be able
to handle that scenario in the first place.

Patricio

pchilano · 2021-03-15T14:10:33Z

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

pchilano · 2021-03-15T14:11:45Z

/integrate

openjdk · 2021-03-15T14:12:49Z

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

80cdf78: 8263544: Unused argument in ConstantPoolCacheEntry::set_field()
c0176c4: 8263552: Use String.valueOf() for char-to-String conversions
fac39fe: 8263508: Remove dead code in MethodHandleImpl
7b4aefe: 8263530: sun.awt.X11.ListHelper.removeAll() should use clear()
32c7fcc: 8263490: [macos] Crash occurs on JPasswordField with activated InputMethod
8afec70: 8260931: Implement JEP 382: New macOS Rendering Pipeline
0638303: 8263497: Clean up sun.security.krb5.PrincipalName::toByteArray
ba22e6f: 8263446: Avoid unary minus over unsigned type in ObjectSynchronizer::dec_in_use_list_ceiling
b371f90: 8263504: Some OutputMachOpcodes fields are uninitialized
f7e0a09: 8263425: AArch64: two potential bugs in C1 LIRGenerator::generate_address()
... and 85 more: https://git.openjdk.java.net/jdk/compare/679faa691adba139c698ec2e34f71f452b6400ad...master

Your commit was automatically rebased without conflicts.

Pushed as commit d6b5e18.

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

v1

e99ca59

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 8, 2021

pchilano marked this pull request as ready for review March 8, 2021 20:52

openjdk bot added the rfr Pull request is ready for review label Mar 8, 2021

coleenp approved these changes Mar 9, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 9, 2021

dholmes-ora approved these changes Mar 9, 2021

View reviewed changes

dcubed-ojdk suggested changes Mar 9, 2021

View reviewed changes

add asserts for thread state + check_asyncs name

865f2d6

dcubed-ojdk suggested changes Mar 11, 2021

View reviewed changes

dcubed-ojdk approved these changes Mar 11, 2021

View reviewed changes

openjdk bot closed this Mar 15, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 15, 2021

8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers #2880

8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers #2880

pchilano commented Mar 8, 2021 •

edited by openjdk bot

bridgekeeper bot commented Mar 8, 2021

openjdk bot commented Mar 8, 2021

mlbridge bot commented Mar 8, 2021 •

edited

coleenp left a comment

openjdk bot commented Mar 9, 2021 •

edited

dholmes-ora left a comment

dcubed-ojdk left a comment

dcubed-ojdk Mar 9, 2021

dcubed-ojdk Mar 9, 2021

dcubed-ojdk Mar 9, 2021

mlbridge bot commented Mar 9, 2021

dcubed-ojdk commented Mar 11, 2021

dcubed-ojdk left a comment

dcubed-ojdk left a comment

mlbridge bot commented Mar 11, 2021

pchilano commented Mar 15, 2021

pchilano commented Mar 15, 2021

openjdk bot commented Mar 15, 2021

8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers #2880

8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers #2880

Conversation

pchilano commented Mar 8, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Mar 8, 2021

openjdk bot commented Mar 8, 2021

mlbridge bot commented Mar 8, 2021 • edited

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 9, 2021 • edited

dholmes-ora left a comment

Choose a reason for hiding this comment

dcubed-ojdk left a comment

Choose a reason for hiding this comment

dcubed-ojdk Mar 9, 2021

Choose a reason for hiding this comment

dcubed-ojdk Mar 9, 2021

Choose a reason for hiding this comment

dcubed-ojdk Mar 9, 2021

Choose a reason for hiding this comment

mlbridge bot commented Mar 9, 2021

dcubed-ojdk commented Mar 11, 2021

dcubed-ojdk left a comment

Choose a reason for hiding this comment

dcubed-ojdk left a comment

Choose a reason for hiding this comment

mlbridge bot commented Mar 11, 2021

pchilano commented Mar 15, 2021

pchilano commented Mar 15, 2021

openjdk bot commented Mar 15, 2021

pchilano commented Mar 8, 2021 •

edited by openjdk bot

mlbridge bot commented Mar 8, 2021 •

edited

openjdk bot commented Mar 9, 2021 •

edited