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

8265932: Move safepoint related fields from class Thread to JavaThread #3695

Closed
wants to merge 3 commits into from

Conversation

@pchilano
Copy link
Contributor

@pchilano pchilano commented Apr 26, 2021

Hi,

Please review the following change. A left over from 8265453 was moving _poll_data from class Thread to JavaThread. There are also other fields like _no_safepoint_count and _visited_for_critical_count that should be moved.
Tested in mach5 tiers 1-5. Tested it builds ppc, arm and s390. Tested it builds in release, optimized and fastdebug modes.

Thanks,
Patricio


Progress

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

Issue

  • JDK-8265932: Move safepoint related fields from class Thread to JavaThread

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3695

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 26, 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 Apr 26, 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.

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Apr 26, 2021

/label remove serviceability

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Apr 26, 2021

/label remove hotspot

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Apr 26, 2021

/label add hotspot-runtime

Loading

@openjdk openjdk bot removed the serviceability label Apr 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 26, 2021

@pchilano
The serviceability label was successfully removed.

Loading

@openjdk openjdk bot removed the hotspot label Apr 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 26, 2021

@pchilano
The hotspot label was successfully removed.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 26, 2021

@pchilano
The hotspot-runtime label was successfully added.

Loading

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

@mlbridge mlbridge bot commented Apr 26, 2021

Webrevs

Loading

Copy link
Contributor

@coleenp coleenp left a comment

Looks good to me!

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 26, 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:

8265932: Move safepoint related fields from class Thread to JavaThread

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

Loading

@openjdk openjdk bot added the ready label Apr 26, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Patricio,

Changes looks good. I had to double-check a few JavaThread conversions actually appear in code that can only be executed by a JavaThread, but they seem okay.

One comment below.

Thanks,
David

Loading

@@ -419,23 +419,6 @@ bool Mutex::contains(Mutex* locks, Mutex* lock) {
return false;
}

// NSV implied with locking allow_vm_block or !safepoint_check locks.
void Mutex::no_safepoint_verifier(Thread* thread, bool enable) {
Copy link
Member

@dholmes-ora dholmes-ora Apr 27, 2021

Choose a reason for hiding this comment

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

Wouldn't it have been simpler to add the is_Java_thread() check in here rather than "inlining" this at the call-sites?

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 27, 2021

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

Hi David,

Thanks for looking at this.

On 4/26/21 9:48 PM, David Holmes wrote:

Hi Patricio,

Changes looks good. I had to double-check a few JavaThread conversions actually appear in code that can only be executed by a JavaThread, but they seem okay.

One comment below.

Thanks,
David

src/hotspot/share/runtime/mutex.cpp line 423:

421:
422: // NSV implied with locking allow_vm_block or !safepoint_check locks.
423: void Mutex::no_safepoint_verifier(Thread* thread, bool enable) {
Wouldn't it have been simpler to add the is_Java_thread() check in here rather than "inlining" this at the call-sites?

Since the call can be replaced by just two lines I thought it was better
to inline it for readability purposes. I also didn't like the name
no_safepoint_verifier() too much since we are not really verifying
anything. The verification is done in check_possible_safepoint(). I now
see the same issue with NoSafepointVerifier (maybe it should be
NoSafepointMark). Anyways, I don't really have a strong opinion about it
so I can restore it if you think it's better with no_safepoint_verifier().

Thanks,
Patricio

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 27, 2021

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

On 27/04/2021 12:48 pm, patricio.chilano.mateo at oracle.com wrote:

Hi David,

Thanks for looking at this.

On 4/26/21 9:48 PM, David Holmes wrote:

Hi Patricio,

Changes looks good. I had to double-check a few JavaThread conversions
actually appear in code that can only be executed by a JavaThread, but
they seem okay.

One comment below.

Thanks,
David

src/hotspot/share/runtime/mutex.cpp line 423:

421:
422: // NSV implied with locking allow_vm_block or !safepoint_check
locks.
423: void Mutex::no_safepoint_verifier(Thread* thread, bool enable) {
Wouldn't it have been simpler to add the is_Java_thread() check in
here rather than "inlining" this at the call-sites?
Since the call can be replaced by just two lines I thought it was better
to inline it for readability purposes. I also didn't like the name
no_safepoint_verifier() too much since we are not really verifying
anything. The verification is done in check_possible_safepoint(). I now
see the same issue with NoSafepointVerifier (maybe it should be
NoSafepointMark). Anyways, I don't really have a strong opinion about it
so I can restore it if you think it's better with no_safepoint_verifier().

The review would have been simpler if this change had not been made, but
now that it has ... I don't feel strongly enough to insist it be restored.

Thanks,
David
-----

Thanks,
Patricio

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 27, 2021

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

On 27/04/2021 12:48 pm, patricio.chilano.mateo at oracle.com wrote:

Hi David,

Thanks for looking at this.

On 4/26/21 9:48 PM, David Holmes wrote:

Hi Patricio,

Changes looks good. I had to double-check a few JavaThread conversions
actually appear in code that can only be executed by a JavaThread, but
they seem okay.

One comment below.

Thanks,
David

src/hotspot/share/runtime/mutex.cpp line 423:

421:
422: // NSV implied with locking allow_vm_block or !safepoint_check
locks.
423: void Mutex::no_safepoint_verifier(Thread* thread, bool enable) {
Wouldn't it have been simpler to add the is_Java_thread() check in
here rather than "inlining" this at the call-sites?
Since the call can be replaced by just two lines I thought it was better
to inline it for readability purposes. I also didn't like the name
no_safepoint_verifier() too much since we are not really verifying
anything. The verification is done in check_possible_safepoint(). I now
see the same issue with NoSafepointVerifier (maybe it should be
NoSafepointMark). Anyways, I don't really have a strong opinion about it
so I can restore it if you think it's better with no_safepoint_verifier().

The review would have been simpler if this change had not been made, but
now that it has ... I don't feel strongly enough to insist it be restored.

Thanks,
David
-----

Thanks,
Patricio

Loading

// the safepoint mechanism.
if (new_owner->is_Java_thread() && _allow_vm_block && this != tty_lock) {
new_owner->as_Java_thread()->inc_no_safepoint_count();
}
Copy link
Contributor

@coleenp coleenp Apr 27, 2021

Choose a reason for hiding this comment

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

I didn't notice this on first pass. I like the change because it was an awkward function anyway because of the enable/disable parameter.

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Apr 27, 2021

Thanks @coleenp and @dholmes-ora for the reviews!

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Apr 27, 2021

/integrate

Loading

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

@openjdk openjdk bot commented Apr 27, 2021

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

  • b67b2b1: 8265690: Use the latest Ubuntu base image version in Docker testing
  • b2628d1: 8263972: C2: LoadVector/StoreVector type mismatch in MemNode::can_see_stored_value()
  • 377b346: 8264752: SIGFPE crash with option FlightRecorderOptions:threadbuffersize=30M

Your commit was automatically rebased without conflicts.

Pushed as commit 5634f20.

💡 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