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

8307153: JVMTI GetThreadState on carrier should return STATE_WAITING #14298

Closed
wants to merge 7 commits into from

Conversation

sspitsyn
Copy link
Contributor

@sspitsyn sspitsyn commented Jun 3, 2023

When a virtual thread is mounted, the carrier thread should be reported as "waiting" until the virtual thread unmounts. Right now, GetThreadState reports a state based the JavaThread status when it should return JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY.
The fix adds:

  • a special case for passive carrier threads
  • necessary test coverage to the existing JVMTI test: serviceability/jvmti/vthread/ThreadStateTest.

Testing:

  • tested with the updated test: serviceability/jvmti/vthread/ThreadStateTest
  • submitted mach5 tiers 1-5
  • TBD: to submit mach5 tier 6

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8307153: JVMTI GetThreadState on carrier should return STATE_WAITING (Bug - "3")

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14298/head:pull/14298
$ git checkout pull/14298

Update a local copy of the PR:
$ git checkout pull/14298
$ git pull https://git.openjdk.org/jdk.git pull/14298/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14298

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14298.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 3, 2023

👋 Welcome back sspitsyn! 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 Jun 3, 2023

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

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jun 3, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 3, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 3, 2023

Webrevs

jint state = get_thread_state_base(thread_oop, jt);

if (is_passive_carrier_thread(jt, thread_oop)) {
state |= (JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is testing if the jt is carrying thread_oop and it's okay for the JVMTI state to reported as WAITING when waiting for something other than Object.wait.

One thing that is a bit confusing is the function name "is_passive_carrier_thread". A platform thread is either a carrier or not. Maybe for a different PR but I think is_passive_carrier_thread should be renamed to avoid the use of the word "passive".

Copy link
Contributor Author

@sspitsyn sspitsyn Jun 4, 2023

Choose a reason for hiding this comment

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

The lines 763-764 are to correct the state exactly for passive carrier thread, a carrier thread which can't progress until the execution control has not been returned from a virtual thread executed on the top. It is never for a platform thread which is not a carrier thread. "Passive" is the best word I was able to find for this meaning. Do you have any other word/suggestion in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

The lines 763-764 are to correct the state exactly for passive carrier thread, a carrier thread which can't progress until the execution control has not been returned from a virtual thread executed on the top. It is never for a platform thread which is not a carrier thread. "Passive" is the best word I was able to find for this meaning. Do you have any other word/suggestion in mind?

It's just a carrier. A platform thread becomes a carrier when a virtual thread is mounted, it ceases to be a carrier once the virtual thread is unmounted. The mental model is that the carrier is blocked so reporting its state as waiting indefinitely is correct. Maybe you don't want to rename it in this PR but renaming this function to something like is_carrying would convey that it's asking the question if a given JavaThread is carrying the given virtual thread oop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I see you point. Unfortunately, I've always referred the platform thread with an executed FJP schedular as a carrier thread. The term 'carrier' with this meaning is everywhere in the JVMTI code. It looks very confusing to call a thread to be a carrier thread only during some phases of its execution.

Copy link
Contributor

@AlanBateman AlanBateman Jun 6, 2023

Choose a reason for hiding this comment

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

Okay, I see you point. Unfortunately, I've always referred the platform thread with an executed FJP schedular as a carrier thread. The term 'carrier' with this meaning is everywhere in the JVMTI code. It looks very confusing to call a thread to be a carrier thread only during some phases of its execution.

Okay, I'm just pointing out that is_passive_carrier_thread looks a bit strange here as it is testing if a JavaThread is carrying a virtual thread oop - it's not testing if the thread is owned by the virtual thread scheduler.

Choose a reason for hiding this comment

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

is_carrying_carrier_thread? a bit artificial, but it's a carrier thread and it's carrying a virtual thread

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_carrying_carrier_thread? a bit artificial, but it's a carrier thread and it's carrying a virtual thread

I guess, your suggestion is is_carrying_virtual_thread. Is it right?
If so, I like this suggestion.

Choose a reason for hiding this comment

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

is_carrying_carrier_thread? a bit artificial, but it's a carrier thread and it's carrying a virtual thread

I guess, your suggestion is is_carrying_virtual_thread. Is it right? If so, I like this suggestion.

Up to you. I think any of this names is better than is_passive_carrier_thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, your suggestion is is_carrying_virtual_thread. Is it right? If so, I like this suggestion.

Good, I think will be easy to understand at the use sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, Alan and Alex.

@plummercj
Copy link
Contributor

Without the fix in place, do your new tests reproduce the issue>

I'm trying to recall the origins of the filing of this CR. I thought I had noticed this issue while working with a JDI test and discussed it with you and Alan. Just wondering if there is something that can be done to a JDI test to also test for this.

@sspitsyn
Copy link
Contributor Author

sspitsyn commented Jun 5, 2023

Without the fix in place, do your new tests reproduce the issue.
I'm trying to recall the origins of the filing of this CR. I thought I had noticed this issue while working with a JDI test and discussed it with you and Alan. Just wondering if there is something that can be done to a JDI test to also test for this.

The updated test is failing without the fix. Unfortunately, I do not remember what failing JDI test you were observed.

@sspitsyn sspitsyn closed this Jun 5, 2023
@sspitsyn sspitsyn reopened this Jun 5, 2023
@@ -382,7 +382,8 @@ class JvmtiEnvBase : public CHeapObj<mtInternal> {
// get carrier thread last java vframe
static javaVFrame* get_cthread_last_java_vframe(JavaThread* jt, RegisterMap* reg_map);

// get ordinary thread thread state
// get platform thread state
static jint get_thread_state_base(oop thread_oop, JavaThread* jt);

Choose a reason for hiding this comment

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

maybe rename it to get_platform_thread_state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it. It will be inconsistent withget_vthread_state.
Ideally, then the get_vthread_state needs to be replaced with the get_virtual_thread_state.

@openjdk
Copy link

openjdk bot commented Jun 6, 2023

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

8307153: JVMTI GetThreadState on carrier should return STATE_WAITING

Reviewed-by: amenkov, cjplummer

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

  • 5722903: 8307374: Add a JFR event for tracking RSS
  • 1de40f3: 8302145: ddepth should be uint in PhaseIdealLoop::register_node()
  • a6726b6: 8309568: javac crashes attempting to -Xprint on a class file of an unnamed class
  • 8cdd95e: 8305959: x86: Improve itable_stub

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 Jun 6, 2023
@sspitsyn
Copy link
Contributor Author

sspitsyn commented Jun 7, 2023

Thank you for review, Alex and Chris.

@sspitsyn
Copy link
Contributor Author

sspitsyn commented Jun 7, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Jun 7, 2023

Going to push as commit 177e832.
Since your change was applied there have been 10 commits pushed to the master branch:

  • f0236ed: 8309543: Micro-optimize x86 assembler UseCondCardMark
  • 9d7bf53: 8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots
  • a1ab377: 8309550: jdk.jfr.internal.Utils::formatDataAmount method should gracefully handle amounts equal to Long.MIN_VALUE
  • c49129f: 8308445: Linker should check that capture state segment is big enough
  • fa79111: 8308031: Linkers should reject unpromoted variadic parameters
  • 16ebf47: 8309594: Cleanup naming in JavacParser related to unnamed classes
  • 5722903: 8307374: Add a JFR event for tracking RSS
  • 1de40f3: 8302145: ddepth should be uint in PhaseIdealLoop::register_node()
  • a6726b6: 8309568: javac crashes attempting to -Xprint on a class file of an unnamed class
  • 8cdd95e: 8305959: x86: Improve itable_stub

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 7, 2023
@openjdk openjdk bot closed this Jun 7, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 7, 2023
@openjdk
Copy link

openjdk bot commented Jun 7, 2023

@sspitsyn Pushed as commit 177e832.

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

@sspitsyn sspitsyn deleted the br39 branch June 7, 2023 13:22
@@ -1728,7 +1741,7 @@ JvmtiEnvBase::suspend_thread(oop thread_oop, JavaThread* java_thread, bool singl
// An attempt to handshake-suspend a passive carrier thread will result in
Copy link
Contributor

Choose a reason for hiding this comment

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

The rename from is_passive_carrier_thread to is_thread_carrying_vthread looks fine. There are a few stray comments that still say "passive carrier thread" that probably should be cleaned up. I see you've just integrated this change but maybe the next change in the area that do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Alan. Will do this cleanup when there is a chance.

@dcubed-ojdk
Copy link
Member

Looks like this PR has caused regression failures in Tier1. We have between
2 and 5 failures per Tier1. See:

JDK-8309612 serviceability/jvmti/vthread/SuspendResume1/SuspendResume1.java#default fails after JDK-8307153

Because this failure is happening in Tier1, combined with the fact that we get much more JVM/TI
testing in the upper Tiers, and tomorrow is the code-fork I'm proceeding with a [BACKOUT] and
am testing that [BACKOUT] with an urgent Tier1 right now.

See:
JDK-8309614 [BACKOUT] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
5 participants