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

8265934: Cleanup _suspend_flags and _special_runtime_exit_condition #3919

Closed
wants to merge 2 commits into from

Conversation

@pchilano
Copy link
Contributor

@pchilano pchilano commented May 7, 2021

Hi,

Please review the following patch which contains the following cleanups:

  • Move _suspend_flags from Thread class to JavaThread

  • Rename _special_runtime_exit_condition to _async_exception_condition. The name has been mixed up with the methods has_special_runtime_exit_condition() and handle_special_runtime_exit_condition() which check both async exception conditions and _suspend_flags. Also rename related methods: clear_special_runtime_exit_condition() -> clear_async_exception_condition(), has_async_condition() -> has_async_exception_condition(). I also added set_async_exception_condition() just for completeness which is now called by set_pending_unsafe_access_error() and set_pending_async_exception().

  • The _has_async_exception enum value in SuspendFlags creates a coupling between _suspend_flags and asynchronous exceptions. This allows us in the transition from native to Java wrappers to do a single load and comparison against the _suspend_flags field instead of having to do one for _suspend_flags and one for asynchronous exceptions. Since this is just an optimization I removed methods has_async_exception(), set_has_async_exception() and clear_has_async_exception() associated with _suspend_flags since they create confusion as to who actually manages asynchronous exceptions. Methods associated with _async_exception_condition should be used instead. I added a comment in set_pending_async_exception() as to why we use _suspend_flags.

  • Remove boolean parameter check_unsafe_error from check_and_handle_async_exceptions(). When we check for async exceptions we always process any async condition except in the transition from native->Java where we skip unsafe access error ones. I moved the boolean parameter to has_async_exception_condition() instead and fixed check_special_condition_for_native_trans(). Method check_and_handle_async_exceptions() could use some further cleanup but I'll do that in another RFR.

Tested in mach5 tiers 1-6.

Thanks,
Patricio


Progress

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

Issue

  • JDK-8265934: Cleanup _suspend_flags and _special_runtime_exit_condition

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3919

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 7, 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 May 7, 2021

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

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented May 7, 2021

/label remove serviceability

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented May 7, 2021

/label remove hotspot

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented May 7, 2021

/label add hotspot-runtime

@openjdk openjdk bot removed the serviceability label May 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@pchilano
The serviceability label was successfully removed.

@openjdk openjdk bot removed the hotspot label May 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@pchilano
The hotspot label was successfully removed.

@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@pchilano
The hotspot-runtime label was successfully added.

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

@mlbridge mlbridge bot commented May 7, 2021

Webrevs

robehn
robehn approved these changes May 11, 2021
Copy link
Contributor

@robehn robehn left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented May 11, 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:

8265934: Cleanup _suspend_flags and _special_runtime_exit_condition

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 May 11, 2021
// Note: this function shouldn't block if it's called in
// _thread_in_native_trans state (such as from
// check_special_condition_for_native_trans()).
void JavaThread::check_and_handle_async_exceptions(bool check_unsafe_error) {
Copy link
Member

@dholmes-ora dholmes-ora May 11, 2021

Choose a reason for hiding this comment

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

I'm not clear how you got rid of the check_unsafe_error. They are a special kind of async exception and the scope of checking for them was supposed to be very narrow.

Copy link
Contributor Author

@pchilano pchilano May 11, 2021

Choose a reason for hiding this comment

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

Hi David,

I'm not clear how you got rid of the check_unsafe_error. They are a special kind of async exception and the scope of checking for them was supposed to be very narrow.

The only time we call check_and_handle_async_exceptions() with check_unsafe_error=false is in check_special_condition_for_native_trans(), in the transition from native to Java, otherwise that argument is always true. I only moved the check to has_async_exception_condition() so for that particular case we only call check_and_handle_async_exceptions() for non _async_unsafe_access_error cases.
That boolean is actually redundant today because in check_special_condition_for_native_trans() we call has_async_exception() first which checks the _suspend_flag bit instead of _special_runtime_exit_condition, and that already only gets set for non _async_unsafe_access_error cases. Then in check_and_handle_async_exceptions() we always check for non _async_unsafe_access_error cases first.

Thanks,
Patricio

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented May 11, 2021

Looks good.
Thanks Robbin!

Patricio

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up.

It would have been helpful to do the code motion as a separate commit
so the rename of things from Thread -> JavaThread would be easily
comparable from baseline -> commit1. Then do the moves of things
from the Thread area of the files to the JavaThread area of the files in
commit2.

I think I was able to follow and check everything by scrolling back
and forth, but I'm not 100% sure.

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented May 11, 2021

Hi Dan,

Thumbs up.

It would have been helpful to do the code motion as a separate commit
so the rename of things from Thread -> JavaThread would be easily
comparable from baseline -> commit1. Then do the moves of things
from the Thread area of the files to the JavaThread area of the files in
commit2.
Right, I didn't thought of that. I'll keep it in mind for future changes. If you want I could try to do it for this one but I would have to force push to my fork after the changes (not sure if it will be handled correctly here).

I think I was able to follow and check everything by scrolling back
and forth, but I'm not 100% sure.
Thanks for the review Dan!

Patricio

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented May 11, 2021

No need to try and redo the PR by force pushing. I'm good with the changes.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Patricio,

Apologies for missing your original explanation about the unsafe-access checking.

The changes seem fine.

Thanks,
David

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented May 13, 2021

Hi David,

Hi Patricio,

Apologies for missing your original explanation about the unsafe-access checking.

The changes seem fine.
Thanks for the review!

Patricio

Thanks,
David

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented May 13, 2021

/integrate

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

@openjdk openjdk bot commented May 13, 2021

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

  • f3c6cda: 8266162: Remove JPackage duplicate tests
  • a259ab4: 8258795: Update IANA Language Subtag Registry to Version 2021-05-11

Your commit was automatically rebased without conflicts.

Pushed as commit 853ffdb.

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

@pchilano pchilano deleted the 8265934 branch Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants