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

8273251: Call check_possible_safepoint() from SafepointMechanism::process_if_requested() #5342

Closed
wants to merge 1 commit into from

Conversation

@pchilano
Copy link
Contributor

@pchilano pchilano commented Sep 1, 2021

Hi,

Please review this small fix to call check_possible_safepoint() from SafepointMechanism::process_if_requested() and remove the same call from transition_and_process(). See the bug description for more detail.
Since there were only two lines left of common code from transition_from_native() and transition_from_vm() I just removed transition_and_process() altogether. This also saves an unneeded extra transition to _thread_in_vm when transitioning from vm to Java.

Testing 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-8273251: Call check_possible_safepoint() from SafepointMechanism::process_if_requested()

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5342

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 1, 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 1, 2021

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

  • hotspot-runtime

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

Loading

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

@mlbridge mlbridge bot commented Sep 1, 2021

Webrevs

Loading

coleenp
coleenp approved these changes Sep 1, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Looks good to me!

Loading

if (AssertWXAtThreadSync) {
assert_wx_state(WXWrite);
}
#endif
Copy link
Contributor

@coleenp coleenp Sep 1, 2021

Choose a reason for hiding this comment

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

I was wondering why you moved this, but it's assert code so it makes sense to me to be with the other assert code. Maybe check_possible_safepoint could have a better name to encompass all these checking and assertions actions but I don't know what better name to give it.
With this code moved here, all the other callers where there's a possible safepoint but one not taken will also check this. Did you test on macosx aarch64 too?

Loading

Copy link
Contributor Author

@pchilano pchilano Sep 1, 2021

Choose a reason for hiding this comment

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

I already run tiers 1-3 and I'm currently running tiers 4-6. Since we already include macosx-aarch64 tasks in those tiers I didn't run any additional tests. I will run an additional test cycle macosx-aarch64 specific. I'm not expecting any failures in that area though since those other places are methods with conditional statements where in one branch we might directly call process_if_requested() already due to some _safepoint_check_always lock. Caller method check_for_valid_safepoint_state() should also be safe since it asserts we are in _thread_in_vm implying WXWrite(in any case the callers of that method could also directly call process_if_requested()). We should probably merge this two methods after I fix callers of process_if_requested() to be in _thread_in_vm.

Loading

Copy link
Member

@dholmes-ora dholmes-ora Sep 2, 2021

Choose a reason for hiding this comment

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

Maybe check_safepoint_ready for the new name? (Not needed for this PR though.)

Loading

Copy link
Contributor Author

@pchilano pchilano Sep 2, 2021

Choose a reason for hiding this comment

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

Sounds good, I'll address that in another RFE then.

Loading

@@ -77,16 +77,6 @@ class ThreadStateTransition : public StackObj {
protected:
JavaThread* _thread;

private:
static inline void transition_and_process(JavaThread *thread, JavaThreadState to, bool check_asyncs) {
Copy link
Contributor

@coleenp coleenp Sep 1, 2021

Choose a reason for hiding this comment

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

👍

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 1, 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:

8273251: Call check_possible_safepoint() from SafepointMechanism::process_if_requested()

Reviewed-by: coleenp, 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 8 new commits pushed to the master branch:

  • 6cfe314: 8272970: Parallelize runtime/InvocationTests/
  • a9a83b2: 8273256: runtime/cds/appcds/TestEpsilonGCWithCDS.java fails due to Unrecognized VM option 'ObjectAlignmentInBytes=64' on x86_32
  • 1a5a2b6: 8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE
  • 2f01a6f: 8273157: Add convenience methods to Messager
  • 9689f61: 8272788: Nonleaf ranked locks should not be safepoint_check_never
  • 4ee0dac: 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs
  • 655ea6d: 8270489: Support archived heap objects in EpsilonGC
  • dacd197: 8273217: Make ParHeapInspectTask _safepoint_check_never

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.

Loading

@openjdk openjdk bot added the ready label Sep 1, 2021
@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Sep 1, 2021

Looks good to me!

Thanks for the review Coleen!

Patricio

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Looks good! Thanks for the cleanup Patricio: fewer functions and no redundant state changes.

Cheers,
David

Loading

if (AssertWXAtThreadSync) {
assert_wx_state(WXWrite);
}
#endif
Copy link
Member

@dholmes-ora dholmes-ora Sep 2, 2021

Choose a reason for hiding this comment

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

Maybe check_safepoint_ready for the new name? (Not needed for this PR though.)

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Sep 2, 2021

Looks good! Thanks for the cleanup Patricio: fewer functions and no redundant state changes.

Thanks for the review David!

Patricio

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Sep 2, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 2, 2021

Going to push as commit 92b05fe.
Since your change was applied there have been 17 commits pushed to the master branch:

  • 29e0f13: 8272385: Enforce ECPrivateKey d value to be in the range [1, n-1] for SunEC provider
  • aaa6f69: 8273250: Address javadoc issues in Deflater::setDictionationary
  • 5ee5dd9: 8272914: Create hotspot:tier2 and hotspot:tier3 test groups
  • 5245c1c: 8273147: Update and restructure TestGCLogMessages log message list
  • 632a7e0: 8273165: GraphKit::combine_exception_states fails with "matching stack sizes" assert
  • c2e015c: 8273229: Update OS detection code to recognize Windows Server 2022
  • 0c1b16b: 8273243: Fix indentations in java.net.InetAddress methods
  • 152e669: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible
  • 857a930: 8263375: Support stack watermarks in Zero VM
  • 6cfe314: 8272970: Parallelize runtime/InvocationTests/
  • ... and 7 more: https://git.openjdk.java.net/jdk/compare/02822e1398d6015f0ed26edd440db8e0d50bf152...master

Your commit was automatically rebased without conflicts.

Loading

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

@openjdk openjdk bot commented Sep 2, 2021

@pchilano Pushed as commit 92b05fe.

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

Loading

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