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

8255384: Remove special_runtime_exit_condition() check from SS::block() #913

Closed
wants to merge 9 commits into from

Conversation

pchilano
Copy link
Contributor

@pchilano pchilano commented Oct 28, 2020

Hi all,

Please review the following patch that removes the call to handle_special_runtime_exit_condition() from SS::block(). This avoids recursive calls when transitioning and stopping for safepoints and also makes the code simpler to read since it is not trivial to deduce why we need to execute the check for certain states but not others, i.e. what exact scenarios we are trying to guard against.

Method handle_special_runtime_exit_condition() checks for external suspends, object deoptimization, async exceptions and JFR suspends. All these need to be checked when transitioning to java and when transitioning out of native (except for async exceptions when transitioning to thread_in_vm). In SS::block() this check is executed for the _thread_new_trans, _thread_in_native_trans and thread_in_Java cases. For _thread_new_trans, we know the JT will have to go through JavaCallWrapper() the first time it transitions to Java and that already has a check for handle_special_runtime_exit_condition(). For _thread_in_native_trans, transitioning out of native already has checks for external suspends, object deoptimization and JFR suspends in check_safepoint_and_suspend_for_native_trans() which is called from ThreadStateTransition::transition_from_native()(called either directly or through the ThreadStateTransition wrappers) and check_special_condition_for_native_trans (for native wrappers case). So that leaves the thread_in_Java case.
Careful analysis shows the handle_special_runtime_exit_condition() call in SS::block() prevents JTs transitioning back to Java from escaping after being externally suspended. This can happen when calling SafepointMechanism::process_if_requested() while transitiong back to java without a later check for external suspend. Looking at the callers of SafepointMechanism::process_if_requested() we see that this can happen from handle_polling_page_exception(), java_suspend_self_with_safepoint_check() and check_safepoint_and_suspend_for_native_trans(). An example of this can be shown for the handle_polling_page_exception() case:
- JT hits a poll exception while executing nmethod.
- JT calls handle_polling_page_exception() ( which doesn't use ThreadStateTransition wrappers) and calls SafepointMechanism::process_if_requested()
- Stops for a safepoint due to a VM_ThreadSuspend request
- Upon unblocking from the safepoint, unless we have the check in SS::block() the JT will transition back to java without actually suspending

The "escape from suspend" scenarios for the other callers of SafepointMechanism::process_if_requested() are described in the comments of the bug as well as the proper fixes.

I have tested the patch several times in mach5 tiers1-7 and saw no issues. Let me know if you think I should run any other special tests.

Thanks,
Patricio


Progress

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

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) (1/9 running) ✔️ (9/9 passed)

Issue

  • JDK-8255384: Remove special_runtime_exit_condition() check from SS::block()

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 28, 2020

👋 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 bot commented Oct 28, 2020

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

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 28, 2020
@pchilano pchilano marked this pull request as ready for review Oct 28, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 28, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 28, 2020

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Patricio,

Thanks for the detailed analysis on this. I agree with what your are doing in pricniple, but disagree with the means you've chosen to do it. Please see comments below.

Thanks,
David

if (self->is_obj_deopt_suspend()) {
self->wait_for_object_deoptimization();
{
ThreadInVMfromJava tivm(self);
Copy link
Member

@dholmes-ora dholmes-ora Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look good to me. We are in low-level safepoint/handshake related code, but we call a higher-level abstraction for thread-state transition management, just to get the side-effect of processing the safepoint/handshake correctly. To me this suggests we're missing an appropriate API entry point to do what needs to be done. I would rather see something like:

SafepointMechanism::process_if_requested(self);
self->handle_special_runtime_exit_condition(true /* check asyncs */);

(assuming that is the correct action of course).

Copy link
Contributor Author

@pchilano pchilano Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, initially I thought of doing that but then decided to use transition wrappers. I'll change it.
I think ideally we would like to use a transition wrapper in SafepointSynchronize::handle_polling_page_exception() but given the complexity of ThreadSafepointState::handle_polling_page_exception() we have to do things more manual unfortunately.

// code will notice the async and deoptimize and the exception will
// be delivered. (Polling at a return point is ok though). Sure is
// a lot of bother for a deprecated feature...
ThreadInVMfromJavaNoAsyncException tivm(self);
Copy link
Member

@dholmes-ora dholmes-ora Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

Copy link
Contributor Author

@pchilano pchilano Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll change it.

/* zap freed handles rather than GC'ing them */ \
HandleMarkCleaner __hmc(THREAD); \
if (SafepointMechanism::should_process(THREAD)) { \
CALL_VM({ThreadInVMfromJava tiv(THREAD);}, handle_exception); \
Copy link
Member

@dholmes-ora dholmes-ora Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially thought this was an okay convenience technique to get the desired effects, but seeing it used elsewhere I no longer think that - see comments "above".

Copy link
Contributor Author

@pchilano pchilano Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I thought using TIVFJ seemed better because when reading CALL_VM() one would expect a transition from _thread_in_Java to _thread_in_vm and then a call to the appropiate method (like with JRT_ENTRY). It's just that in this case the call that we want to make (SafepointMechanism::process_if_requested()) is already included in the TIVFJ. Anyways, doing it the other way is okay too.

@pchilano
Copy link
Contributor Author

pchilano commented Oct 29, 2020

Hi Patricio,

Thanks for the detailed analysis on this. I agree with what your are doing in pricniple, but disagree with the means you've chosen to do it. Please see comments below.

Thanks,
David

Hi David,

Please check the updated version. I added also a has_special_runtime_exit_condition() check before actually calling handle_special_runtime_exit_condition().

Patricio

Copy link
Member

@dholmes-ora dholmes-ora left a comment

This seems functionally fine, but it still seems to me that we are missing a Safepoint or SafepointMechanism API helper method that does:

SafepointMechanism::process_if_requested(THREAD); 
if (THREAD->has_special_runtime_exit_condition()) { 
    THREAD->handle_special_runtime_exit_condition(checkAsyncs); 
}

Lets see if others have an opinion.
Thanks.

if (self->is_obj_deopt_suspend()) {
self->wait_for_object_deoptimization();
if (self->has_special_runtime_exit_condition()) {
self->handle_special_runtime_exit_condition();
Copy link
Member

@dholmes-ora dholmes-ora Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add explicit arg: "(true /* check asyncs */)"

Copy link
Contributor Author

@pchilano pchilano Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

if (SafepointMechanism::should_process(THREAD)) { \
CALL_VM(SafepointMechanism::process_if_requested(THREAD); \
if (THREAD->has_special_runtime_exit_condition()) { \
THREAD->handle_special_runtime_exit_condition(); \
Copy link
Member

@dholmes-ora dholmes-ora Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add explicit arg: "(true /* check asyncs */)"

Using a multi-statement code block for the CALL_VM macro looks odd to me, but I find these zero macros unpleasant to begin with.

Copy link
Contributor Author

@pchilano pchilano Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's also why I liked better the TIVFJ wrapper. But if we can agree on the helper method then that would change to a single line.

@pchilano
Copy link
Contributor Author

pchilano commented Nov 3, 2020

This seems functionally fine, but it still seems to me that we are missing a Safepoint or SafepointMechanism API helper method that does:

SafepointMechanism::process_if_requested(THREAD); 
if (THREAD->has_special_runtime_exit_condition()) { 
    THREAD->handle_special_runtime_exit_condition(checkAsyncs); 
}

Lets see if others have an opinion.
Thanks.

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

Copy link
Contributor

@reinrich reinrich left a comment

Hi Patricio,

the change looks good to me.

In JavaThread::check_and_handle_async_exceptions() the block depending on is_at_poll_safepoint() looks like dead code now. I wonder if ThreadSafepointState::_at_poll_safepoint could even be DEBUG_ONLY?

Thanks, Richard.

@mlbridge
Copy link

mlbridge bot commented Nov 3, 2020

Mailing list message from Patricio Chilano on hotspot-runtime-dev:

Hi Richard,

On 11/3/20 12:22 PM, Richard Reingruber wrote:

On Thu, 29 Oct 2020 22:10:58 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

Hi all,

Please review the following patch that removes the call to handle_special_runtime_exit_condition() from SS::block(). This avoids recursive calls when transitioning and stopping for safepoints and also makes the code simpler to read since it is not trivial to deduce why we need to execute the check for certain states but not others, i.e. what exact scenarios we are trying to guard against.

Method handle_special_runtime_exit_condition() checks for external suspends, object deoptimization, async exceptions and JFR suspends. All these need to be checked when transitioning to java and when transitioning out of native (except for async exceptions when transitioning to thread_in_vm). In SS::block() this check is executed for the _thread_new_trans, _thread_in_native_trans and thread_in_Java cases. For _thread_new_trans, we know the JT will have to go through JavaCallWrapper() the first time it transitions to Java and that already has a check for handle_special_runtime_exit_condition(). For _thread_in_native_trans, transitioning out of native already has checks for external suspends, object deoptimization and JFR suspends in check_safepoint_and_suspend_for_native_trans() which is called from ThreadStateTransition::transition_from_native()(called either directly or through the ThreadStateTransition wrappers) and check_special_condition_for_native_trans (for native w!
rappers c
ase). So that leaves the thread_in_Java case.
Careful analysis shows the handle_special_runtime_exit_condition() call in SS::block() prevents JTs transitioning back to Java from escaping after being externally suspended. This can happen when calling SafepointMechanism::process_if_requested() while transitiong back to java without a later check for external suspend. Looking at the callers of SafepointMechanism::process_if_requested() we see that this can happen from handle_polling_page_exception(), java_suspend_self_with_safepoint_check() and check_safepoint_and_suspend_for_native_trans(). An example of this can be shown for the handle_polling_page_exception() case:
- JT hits a poll exception while executing nmethod.
- JT calls handle_polling_page_exception() ( which doesn't use ThreadStateTransition wrappers) and calls SafepointMechanism::process_if_requested()
- Stops for a safepoint due to a VM_ThreadSuspend request
- Upon unblocking from the safepoint, unless we have the check in SS::block() the JT will transition back to java without actually suspending

The "escape from suspend" scenarios for the other callers of SafepointMechanism::process_if_requested() are described in the comments of the bug as well as the proper fixes.

I have tested the patch several times in mach5 tiers1-7 and saw no issues. Let me know if you think I should run any other special tests.

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

Make direct calls instead of using transition wrappers
Hi Patricio,

the change looks good to me.

Thanks for looking at this.

In `JavaThread::check_and_handle_async_exceptions()` the block depending on is_at_poll_safepoint() looks like dead code now. I wonder if `ThreadSafepointState::_at_poll_safepoint` could even be DEBUG_ONLY?

Yes, I actually thought about doing that in the first version but then I
realized that code was already dead even before this change. We only
call set_at_poll_safepoint() in handle_polling_page_exception() and the
handle_special_runtime_exit_condition() call in SS::block() already
excludes checking async exceptions for that case. The call I removed
from ~TIVMFH was exactly the same. So I don't see a path where it could
be called where is_at_poll_safepoint() returned true.
I agree that _at_poll_safepoint should probably be DEBUG_ONLY. Then we
should add an assert in check_and_handle_async_exceptions(). Do you
think I should do that here or in another bug?

Thanks,
Patricio

@reinrich
Copy link
Contributor

reinrich commented Nov 3, 2020

In JavaThread::check_and_handle_async_exceptions() the block depending on is_at_poll_safepoint() looks like dead code now. I wonder if ThreadSafepointState::_at_poll_safepoint could even be DEBUG_ONLY?

Yes, I actually thought about doing that in the first version but then I
realized that code was already dead even before this change. We only
call set_at_poll_safepoint() in handle_polling_page_exception() and the
handle_special_runtime_exit_condition() call in SS::block() already
excludes checking async exceptions for that case. The call I removed
from ~TIVMFH was exactly the same. So I don't see a path where it could
be called where is_at_poll_safepoint() returned true.
I agree that _at_poll_safepoint should probably be DEBUG_ONLY. Then we
should add an assert in check_and_handle_async_exceptions(). Do you
think I should do that here or in another bug?

I'd think you can do it in another bug also. I'm ok either way actually.

Thanks, Richard.

@pchilano
Copy link
Contributor Author

pchilano commented Nov 3, 2020

In JavaThread::check_and_handle_async_exceptions() the block depending on is_at_poll_safepoint() looks like dead code now. I wonder if ThreadSafepointState::_at_poll_safepoint could even be DEBUG_ONLY?

Yes, I actually thought about doing that in the first version but then I
realized that code was already dead even before this change. We only
call set_at_poll_safepoint() in handle_polling_page_exception() and the
handle_special_runtime_exit_condition() call in SS::block() already
excludes checking async exceptions for that case. The call I removed
from ~TIVMFH was exactly the same. So I don't see a path where it could
be called where is_at_poll_safepoint() returned true.
I agree that _at_poll_safepoint should probably be DEBUG_ONLY. Then we
should add an assert in check_and_handle_async_exceptions(). Do you
think I should do that here or in another bug?

I'd think you can do it in another bug also. I'm ok either way actually.
Ok, I filed 8255849 to track that.
Thanks Richard!

Patricio

@mlbridge
Copy link

mlbridge bot commented Nov 3, 2020

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

On 4/11/2020 3:02 am, Patricio Chilano Mateo wrote:

On Tue, 3 Nov 2020 02:32:07 GMT, David Holmes <dholmes at openjdk.org> wrote:

This seems functionally fine, but it still seems to me that we are missing a Safepoint or SafepointMechanism API helper method that does:

```
SafepointMechanism::process_if_requested(THREAD);
if (THREAD->has_special_runtime_exit_condition()) {
THREAD->handle_special_runtime_exit_condition(checkAsyncs);
}
```

Lets see if others have an opinion.
Thanks.

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

That works for me.

Thanks,
David
-----

@robehn
Copy link
Contributor

robehn commented Nov 4, 2020

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

But _suspend_flag have nothing to do with safepoint polling, handling it with SafepointMechanism doesn't seem right?

Thanks, Robbin

That works for me.

Thanks,
David

@pchilano
Copy link
Contributor Author

pchilano commented Nov 4, 2020

Hi Robbin,

Thanks, for looking at this.

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

But _suspend_flag have nothing to do with safepoint polling, handling it with SafepointMechanism doesn't seem right?
Yes, but today we are already checking the _suspend_flag cases inside SS::block(), so this would only add a method to explicitly do that if you want, i.e. make it visible. In any case, I'm fine with both ways of doing it. I can leave as it is right now and see if David is fine with that.

Patricio

Thanks, Robbin

That works for me.

Thanks,

David

@robehn
Copy link
Contributor

robehn commented Nov 4, 2020

Hi Robbin,

Thanks, for looking at this.

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

But _suspend_flag have nothing to do with safepoint polling, handling it with SafepointMechanism doesn't seem right?
Yes, but today we are already checking the _suspend_flag cases inside SS::block(), so this would only add a method to explicitly do that if you want, i.e. make it visible. In any case, I'm fine with both ways of doing it. I can leave as it is right now and see if David is fine with that.

Patricio

Thanks, Robbin

That works for me.

Thanks,

David

Skimming a bit, as far as I can see void ThreadSafepointState::handle_polling_page_exception() should use ThreadInVMfromJava and in the other path ThreadInVMfromJavaNoAsyncException.
No?

@pchilano
Copy link
Contributor Author

pchilano commented Nov 4, 2020

Hi Robbin,
Thanks, for looking at this.

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

But _suspend_flag have nothing to do with safepoint polling, handling it with SafepointMechanism doesn't seem right?
Yes, but today we are already checking the _suspend_flag cases inside SS::block(), so this would only add a method to explicitly do that if you want, i.e. make it visible. In any case, I'm fine with both ways of doing it. I can leave as it is right now and see if David is fine with that.

Patricio

Thanks, Robbin

That works for me.

Thanks,

David

Skimming a bit, as far as I can see void ThreadSafepointState::handle_polling_page_exception() should use ThreadInVMfromJava and in the other path ThreadInVMfromJavaNoAsyncException.
No?
Yes, that was actually v1 but then I agreed with David that it was better to make explicit calls. Let me know though if you are still okay with this version. I see that you want to properly transition to vm before going to thread_blocked but as we discussed offline we will need more work to fix all instances of that. : )

Patricio

@mlbridge
Copy link

mlbridge bot commented Nov 4, 2020

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

On 5/11/2020 12:26 am, Robbin Ehn wrote:

On Wed, 4 Nov 2020 14:05:52 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

Hi Robbin,

Thanks, for looking at this.

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

But _suspend_flag have nothing to do with safepoint polling, handling it with SafepointMechanism doesn't seem right?
Yes, but today we are already checking the _suspend_flag cases inside SS::block(), so this would only add a method to explicitly do that if you want, i.e. make it visible. In any case, I'm fine with both ways of doing it. I can leave as it is right now and see if David is fine with that.

Patricio

Thanks, Robbin

That works for me.
## Thanks,
David

Skimming a bit, as far as I can see void ThreadSafepointState::handle_polling_page_exception() should use ThreadInVMfromJava and in the other path ThreadInVMfromJavaNoAsyncException.
No?

No :) I strongly object to using dummy thread-state transitions just to
get the side-effects that are part of real thread-state transitions. If
you need that side-effect in this code then it should be explicit in
this code.

And when we have a recurring pattern like:

SafepointMechanism::process_if_requested(THREAD);
if (THREAD->has_special_runtime_exit_condition()) {
THREAD->handle_special_runtime_exit_condition(checkAsyncs);
}

it says to me that we are missing an appropriate API abstraction to
capture why we need these checks in the calling code.

Whether there are things presently inside
handle_special_runtime_exit_condition that are not relevant to these
calls sites (eg. suspend flag) is a different issue.

Cheers,
David

@pchilano
Copy link
Contributor Author

pchilano commented Nov 5, 2020

@robehn are you okay with SafepointMechanism::process_if_requested_with_exit_check()?

Patricio

@robehn
Copy link
Contributor

robehn commented Nov 5, 2020

@robehn are you okay with SafepointMechanism::process_if_requested_with_exit_check()?

Patricio

Yes, with a caveat; I may try to move it later.

SafepointMechanism::process_if_requested(THREAD);
if (THREAD->has_special_runtime_exit_condition()) {
THREAD->handle_special_runtime_exit_condition(checkAsyncs);
}

it says to me that we are missing an appropriate API abstraction to
capture why we need these checks in the calling code.

Yes in this is what interfaceSupport.xxx gives us.
So I would had preferred a method in there or use transition RAII thingies.
(I argue we would not be using them for side effect, because you should to be in VM to be blocked.
E.g. blocked in native makes no sense and we have no 'blocked' in java.
So to me this is just a place where cheat transition because we can get away with it.)

Whether there are things presently inside
handle_special_runtime_exit_condition that are not relevant to these
calls sites (eg. suspend flag) is a different issue.

Cheers,
David

@pchilano
Copy link
Contributor Author

pchilano commented Nov 5, 2020

@robehn are you okay with SafepointMechanism::process_if_requested_with_exit_check()?
Patricio

Yes, with a caveat; I may try to move it later.

Ok, sounds good. I'll update.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Okay. It took me some time to be convinced of the equivalence,
but I'm on-board at this point. The new code is much more clear
and easy to reason about (once you convince yourself that the
new, simpler code is equivalent to the older code).

As @dholmes-ora likes to say: the proof is in the testing. I'll add
that the proof will be in the stress testing.

if (self->is_obj_deopt_suspend()) {
self->wait_for_object_deoptimization();
}
Copy link
Member

@dcubed-ojdk dcubed-ojdk Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why this if-block is deleted.
You change to call SafepointMechanism::process_if_requested_with_exit_check(),
but I don't see equivalent code there.

Update: I found it in JavaThread::handle_special_runtime_exit_condition().

if (self->is_obj_deopt_suspend()) {
self->wait_for_object_deoptimization();
}
Copy link
Member

@dcubed-ojdk dcubed-ojdk Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why this if-block is deleted.
You change to call SafepointMechanism::process_if_requested_with_exit_check(),
but I don't see equivalent code there.

Update: I found it in JavaThread::handle_special_runtime_exit_condition().

}
// We never deliver an async exception at a polling point as the
// compiler may not have an exception handler for it. The polling
// code will notice the async and deoptimize and the exception will
Copy link
Member

@dcubed-ojdk dcubed-ojdk Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a little rewording:

code will notice the pending async exception, deoptimize and the exception will

and then reformat the block comment a bit.

Copy link
Contributor Author

@pchilano pchilano Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -80,7 +80,7 @@ void SafepointMechanism::process(JavaThread *thread) {
// Any load in ::block must not pass the global poll load.
// Otherwise we might load an old safepoint counter (for example).
OrderAccess::loadload();
SafepointSynchronize::block(thread); // Recursive
Copy link
Member

@dcubed-ojdk dcubed-ojdk Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't that comment just added recently? :-)

Copy link
Contributor Author

@pchilano pchilano Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

: ) Should have been there before that!

if (thread->is_obj_deopt_suspend()) {
thread->wait_for_object_deoptimization();
}
Copy link
Member

@dcubed-ojdk dcubed-ojdk Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why this if-block is deleted.
You change to call SafepointMechanism::process_if_requested_with_exit_check(),
but I don't see equivalent code there.

Update: I found it in JavaThread::handle_special_runtime_exit_condition().

thread->wait_for_object_deoptimization();
}

JFR_ONLY(SUSPEND_THREAD_CONDITIONAL(thread);)
Copy link
Member

@dcubed-ojdk dcubed-ojdk Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where's the equivalent of this JFR code?

Update: I found it in JavaThread::handle_special_runtime_exit_condition().

if (thread->is_external_suspend()) {
thread->java_suspend_self_with_safepoint_check();
Copy link
Member

@dcubed-ojdk dcubed-ojdk Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the equivalent of this part of the if-statement in
JavaThread::handle_special_runtime_exit_condition().

@openjdk
Copy link

openjdk bot commented Nov 5, 2020

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

8255384: Remove special_runtime_exit_condition() check from SS::block()

Reviewed-by: dholmes, rrich, 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 1 new commit pushed to the master branch:

  • f611fdf: 8254016: Test8237524 fails with -XX:-CompactStrings option

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 Pull request is ready to be integrated label Nov 5, 2020
@pchilano
Copy link
Contributor Author

pchilano commented Nov 5, 2020

Okay. It took me some time to be convinced of the equivalence,
but I'm on-board at this point. The new code is much more clear
and easy to reason about (once you convince yourself that the
new, simpler code is equivalent to the older code).

As @dholmes-ora likes to say: the proof is in the testing. I'll add
that the proof will be in the stress testing.
Sounds good. So far I tested it a lot in mach5 and locally and will run a last round before pushing. : )

Thanks Dan!

@mlbridge
Copy link

mlbridge bot commented Nov 5, 2020

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

On 6/11/2020 4:19 am, Robbin Ehn wrote:

On Wed, 4 Nov 2020 14:24:00 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

Hi Robbin,

Thanks, for looking at this.

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

But _suspend_flag have nothing to do with safepoint polling, handling it with SafepointMechanism doesn't seem right?
Yes, but today we are already checking the _suspend_flag cases inside SS::block(), so this would only add a method to explicitly do that if you want, i.e. make it visible. In any case, I'm fine with both ways of doing it. I can leave as it is right now and see if David is fine with that.

Patricio

Thanks, Robbin

That works for me.
## Thanks,
David

Hi Robbin,

Thanks, for looking at this.

How about SafepointMechanism::process_if_requested_with_exit_check(bool check_asyncs)?

But _suspend_flag have nothing to do with safepoint polling, handling it with SafepointMechanism doesn't seem right?
Yes, but today we are already checking the _suspend_flag cases inside SS::block(), so this would only add a method to explicitly do that if you want, i.e. make it visible. In any case, I'm fine with both ways of doing it. I can leave as it is right now and see if David is fine with that.

Patricio

Thanks, Robbin

That works for me.
## Thanks,
David

Skimming a bit, as far as I can see void ThreadSafepointState::handle_polling_page_exception() should use ThreadInVMfromJava and in the other path ThreadInVMfromJavaNoAsyncException.
No?

@robehn are you okay with SafepointMechanism::process_if_requested_with_exit_check()?

Patricio

Yes, with a caveat; I may try to move it later.

SafepointMechanism::process_if_requested(THREAD);
if (THREAD->has_special_runtime_exit_condition()) {
THREAD->handle_special_runtime_exit_condition(checkAsyncs);
}

it says to me that we are missing an appropriate API abstraction to
capture why we need these checks in the calling code.

Yes in this is what interfaceSupport.xxx gives us.
So I would had preferred a method in there or use transition RAII thingies.
(I argue we would not be using them for side effect, because you should to be in VM to be blocked.
E.g. blocked in native makes no sense and we have no 'blocked' in java.
So to me this is just a place where cheat transition because we can get away with it.)

I don't understand what you are referring to here with reference to
being blocked? The version 1 code used the transition helpers in a
standalone/isolated way e.g.:

{
ThreadInVMfromJava tivm;
}

which transitions from Java to VM and back, for absolutely no reason
other than to get the side-effects of doing those transitions.

David
-----

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Thanks for the updates! All LGTM.

David

@pchilano
Copy link
Contributor Author

pchilano commented Nov 5, 2020

Thanks for the updates! All LGTM.

David
Great, thanks David!

Patricio

@robehn
Copy link
Contributor

robehn commented Nov 6, 2020

{
ThreadInVMfromJava tivm;
}

which transitions from Java to VM and back, for absolutely no reason
other than to get the side-effects of doing those transitions.

As I see it, if you have an oop map and are executing in VM, you are technically in VM.
But here we skip going to VM since SS:block used to need to know where you are going back to due to suspend flag is handle inside SS:block().
And the safepoint synchronizer will treat java/vm the same thus we can cheat.

If the thread is in Java it is not guaranteed that it will have an oop map and it's not certain that it can be made walkable.
Being blocked requires you to be walkable, thus going from java->blocked cannot be done as general rule.
Instead of having a ton of special rules which no one can remember, the rule is you can only block in VM where we know you can be made walkable.

Polling page exception handling can thus been seen as a cheat avoiding setting in vm since it will actually have no effect on the safepoint synchronizer.

So no we are not looking for side-effects, we need to be in VM (at minimum technically) to become blocked.
Thus the rules for going from VM to Java applies, and we should deliver e.g. async exceptions.

{
ThreadInVMfromJava tivm;
SS:block()
}

Would be the preferred way of doing it.

David

@mlbridge
Copy link

mlbridge bot commented Nov 9, 2020

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

Hi Robbin,

On 6/11/2020 6:39 pm, Robbin Ehn wrote:

David wrote:

{
ThreadInVMfromJava tivm;
}

which transitions from Java to VM and back, for absolutely no reason
other than to get the side-effects of doing those transitions.

As I see it, if you have an oop map and are executing in VM, you are technically in VM.
But here we skip going to VM since SS:block used to need to know where you are going back to due to suspend flag is handle inside SS:block().
And the safepoint synchronizer will treat java/vm the same thus we can cheat.

If the thread is in Java it is not guaranteed that it will have an oop map and it's not certain that it can be made walkable.
Being blocked requires you to be walkable, thus going from java->blocked cannot be done as general rule.
Instead of having a ton of special rules which no one can remember, the rule is you can only block in VM where we know you can be made walkable.

Sorry but I don't understand the point you are making here. The
transition from Java to VM does nothing except change the thread-state
and so has no affect on whether a thread can be made walkable or not.
And SS:block() makes the thread walkable in any case.

Polling page exception handling can thus been seen as a cheat avoiding setting in vm since it will actually have no effect on the safepoint synchronizer.

So no we are not looking for side-effects, we need to be in VM (at minimum technically) to become blocked.

But that is simply not true - we end up being blocked for
safepoints/handshakes from a wide-range of thread states.

Thus the rules for going from VM to Java applies, and we should deliver e.g. async exceptions.

{
ThreadInVMfromJava tivm;
SS:block()
}

Would be the preferred way of doing it.

??? Why would you call SS::block explicitly there - the transition back
to VM would do that.

David
-----

@robehn
Copy link
Contributor

robehn commented Nov 9, 2020

Hi David,

And the safepoint synchronizer will treat java/vm the same thus we can cheat.
If the thread is in Java it is not guaranteed that it will have an oop map and it's not certain that it can be made walkable.
Being blocked requires you to be walkable, thus going from java->blocked cannot be done as general rule.
Instead of having a ton of special rules which no one can remember, the rule is you can only block in VM where we know you can be made walkable.

Sorry but I don't understand the point you are making here. The
transition from Java to VM does nothing except change the thread-state
and so has no affect on whether a thread can be made walkable or not.
And SS:block() makes the thread walkable in any case.

We have many such transition which only sets correct thread state.

Polling page exception handling can thus been seen as a cheat avoiding setting in vm since it will actually have no effect on the safepoint synchronizer.
So no we are not looking for side-effects, we need to be in VM (at minimum technically) to become blocked.

Either you can look this as set of rules that we can follow.
Where at some places we have choose to optimize away some parts.
Or everything in the code is just ad-hoc and special cases.

But that is simply not true - we end up being blocked for
safepoints/handshakes from a wide-range of thread states.

Thus the rules for going from VM to Java applies, and we should deliver e.g. async exceptions.
{
ThreadInVMfromJava tivm;
SS:block()
}
Would be the preferred way of doing it.

??? Why would you call SS::block explicitly there - the transition back
to VM would do that.

The point being that we should be in VM when entering SS:block().
The rules for being in VM from applies:
~ You must be walkable, no _suspend_flag check is needed.
And the same when going back from VM back to java:
~ Deliver all _suspend_flag events.

So you can either see this "special code".
Or, we have just removed redundant code, other than that this
follows the 'generic rules'.

It is much easier to read/review the code as:
Java->VM, oopmap, no front edge polling/_suspend_flag, no fence
VM->Blocked, walkable, no front edge polling/_suspend, no fence
blocked
Blocked->VM, back edges polling, no suspend_flag, fence
VM->Java, back edges polling, all suspend_flag big, no fence

When setting thread state without fence we may skip it, front edge of going Java->VM->Blocked can be optimized to:

  • walkable
  • set blocked

Back edge can be optimized to:

  • set any unsafe state (e.g. vm/block_trans) + fence
  • check poll, check suspend_flag
  • set thread state to Java

This is what we do, but it is distributed several methods ~3-4 methods.

So the reason for ending-up in SS:block with all kinds of state is that in some-cases the first part of the transition is in another methods e.g. the native wrapper.

That case can be seen as native->vm->blocked->vm->java:

Native->VM, oopmap, front edge polling/_suspend_flag, fence (safe>unsafe)
VM->Blocked, walkable, no front edge polling/_suspend, no fence, unsafe->safe
blocked
Blocked->VM, back edges polling, no suspend_flag, fence
VM->Java, back edges polling, all suspend_flag big, no fence

The native wrapper does the front edge polling safe->unsafe + fence
Since native_trans is also an unsafe state and going to another unsafe state does not require a fence, we can just optimize that away and just set blocked directly, so optimized it looks like:

  • native wrapper
  • walkable
  • set blocked

When going back it looks the other case of blocked->vm->java, but native wrapper sets the final thread state of thread in Java.

  • set any unsafe state (e.g. any _trans state) + fence
  • check poll, check suspend_flag
  • set thread state to Java done by wrapper in this case

You don't have to think of it like this, but it certainly helps me when writing/reviewing the code.

Thanks, Robbin

David

@pchilano
Copy link
Contributor Author

pchilano commented Nov 16, 2020

/integrate

@openjdk openjdk bot closed this Nov 16, 2020
@openjdk 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 Nov 16, 2020
@openjdk
Copy link

openjdk bot commented Nov 16, 2020

@pchilano Since your change was applied there has been 1 commit pushed to the master branch:

  • f611fdf: 8254016: Test8237524 fails with -XX:-CompactStrings option

Your commit was automatically rebased without conflicts.

Pushed as commit 3675653.

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

openjdk-notifier bot referenced this pull request Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
5 participants