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

8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING #14366

Closed
wants to merge 6 commits into from

Conversation

sspitsyn
Copy link
Contributor

@sspitsyn sspitsyn commented Jun 7, 2023

This is REDO the fix of JDK-8307153.
The last update of the fix in the review cycle was incorrect and incorrectly tested, so the issue has not been noticed. It is why the fix was backed out.
The issue is that the SUSPEND bit was missed in the JVMTI thread state of platform/carrier threads carrying virtual threads (seeJvmtiEnvBase::get_thread_state function).

The first push/patch is the original fix of JDK-8307153.
The fix of the SUSPEND bit issue will be in the incremental update.
It is to simplify the review.

Testing:

  • TBD: mach5 tiers 1-5

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-8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING (Bug - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14366

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 7, 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 openjdk bot added the rfr Pull request is ready for review label Jun 7, 2023
@openjdk
Copy link

openjdk bot commented Jun 7, 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 7, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 7, 2023

Webrevs

@sspitsyn sspitsyn changed the title 8309612: [R[REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING Jun 7, 2023
@openjdk openjdk bot removed the rfr Pull request is ready for review label Jun 7, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 7, 2023
src/hotspot/share/prims/jvmtiEnvBase.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiEnvBase.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiEnvBase.hpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiEnvBase.hpp Outdated Show resolved Hide resolved
Comment on lines 764 to 765
state &= ~JVMTI_THREAD_STATE_RUNNABLE;
state |= JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY;

Choose a reason for hiding this comment

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

This does not look correct.
GetThreadState spec provides hierarchical set of questions to interpret thread state value.
JVMTI_THREAD_STATE_ALIVE | JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY is only one branch and I'd expect all other bits are not set for this state.
Need to decide what do we want to report as carrier thread state for all possible values returned by get_thread_state_base().

Copy link
Contributor Author

@sspitsyn sspitsyn Jun 7, 2023

Choose a reason for hiding this comment

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

Good concern.
There are two bits (and the related RUNNABLE bit) that we care in this sub-tree of state bits: SUSPENDED and INTERRUPTED. This update clones these two bits. The RUNNABLE bit must be cleared.
A thread carrying a virtual thread can not be in native, blocked, parked, sleeping or waiting on some object.
The state returned by the get_thread_state_base is based on the call:
state = (jint)java_lang_Thread::get_thread_status(thread_oop);
and addition of the derived from JavaThread bits: SUSPENDED, INTERRUPTED and IN_NATIVE.
The three bits derived from the JavaThread are not relevant.
This call has to be made directly:
state = (jint)java_lang_Thread::get_thread_status(thread_oop);
The SUSPEND bit has to be based on the call:
jt->is_carrier_thread_suspended();

The function get_thread_state will look as below:

  if (is_thread_carrying_vthread(jt, thread_oop)) {
    jint state = (jint)java_lang_Thread::get_thread_status(thread_oop);
    if (jt->is_carrier_thread_suspended()) {
      state |= JVMTI_THREAD_STATE_SUSPENDED;
    }
    // It's okay for the JVMTI state to be reported as WAITING when waiting
    // for something other than an Object.wait. So, we treat a thread carrying
    // a virtual thread as waiting indefinitely which is not runnable.
    // It is why the RUNNABLE bit is cleared and the WAITING bits are added.
    state &= ~JVMTI_THREAD_STATE_RUNNABLE;
    state |= JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY;
    return state;
  } else { 
    return get_thread_state_base(thread_oop, jt);
  }

Copy link

@alexmenkov alexmenkov Jun 7, 2023

Choose a reason for hiding this comment

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

Do you need to check jt->is_interrupted(false) and set INTERRUPTED bit?
It looks like java_lang_Thread::get_thread_status(thread_oop) can only return RUNNABLE in the case and we clear it, so the call is not needed:

if (is_thread_carrying_vthread(jt, thread_oop)) {
  jint state = JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY;
  if (jt->is_carrier_thread_suspended()) {
    state |= JVMTI_THREAD_STATE_SUSPENDED;
  }
  if (jt->is_interrupted(false)) {
    state |= JVMTI_THREAD_STATE_INTERRUPTED;
  }
  return state;
} else ...

Choose a reason for hiding this comment

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

A thread carrying a virtual thread can not be in native, blocked, parked, sleeping or waiting on some object.

Actually it can be in native.
And if I remember correctly synchronized block pins virtual thread, so inside synchronized we can get other states

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The INTERRUPTED bit we need has to be returned by the java_lang_Thread::get_thread_status.
Not completely sure but the bit jt->is_interrupted(false) can be set for the mounted virtual thread.
The JVMTI InterruptThread calls this function to set interrupt bit for non-virtual threads:
java_lang_Thread::set_interrupted(thread_obj, true);

Copy link
Contributor Author

@sspitsyn sspitsyn Jun 8, 2023

Choose a reason for hiding this comment

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

A thread carrying a virtual thread can not be in native, blocked, parked, sleeping or waiting on some object.

A virtual thread can call native code, be blocked on an object monitor, or waiting on an object monitor. Only parking and sleeping are specialized for virtual threads in the list you gave.

This statement was about a carrier thread (not a JavaThread and not a java.lang.VirtualThread) when there is a virtual thread executed at the top. We are getting state bits with the java_lang_Thread::get_thread_status(thread_oop) where the thread_oop belongs to the carrier thread. But you are talking about a virtual thread which, of course, can be in almost any state.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying - it gets very confusing as to which "thread" is being talked about. But if a virtual thread is mounted on this JavaThread then I thought the carrier thread's thread-oop is supposed to be in a blocked state?

Copy link
Contributor Author

@sspitsyn sspitsyn Jun 8, 2023

Choose a reason for hiding this comment

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

It was decided with Alan that it is okay to be in a waiting state. The JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER state requires a monitor to be blocked on, so it can be confusing. Alan's comment in the original PR #14298 was:

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The mental model is that the carrier is blocked so this is what an observer using the APIs should see. My recollection is that JVMTI_THREAD_STATE_WAITING was okay because there is a wriggle room in the JVM TI spec, it only uses Object.wait as an example. There may be a few rough edges to smooth down in this area. It's okay to take time with this PR and expand the tests to cover more cases and get more confident that there aren't more issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We agreed with Alex to file a test RFE to improve test coverage in this area.

@openjdk
Copy link

openjdk bot commented Jun 7, 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:

8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING

Reviewed-by: cjplummer, amenkov

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 24 new commits pushed to 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 7, 2023
@sspitsyn
Copy link
Contributor Author

sspitsyn commented Jun 9, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Jun 9, 2023

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

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jun 9, 2023

@sspitsyn Pushed as commit f91e9ba.

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

@sspitsyn sspitsyn deleted the br42 branch June 9, 2023 06:15
@sspitsyn sspitsyn restored the br42 branch June 9, 2023 06:15
@sspitsyn
Copy link
Contributor Author

sspitsyn commented Jun 9, 2023

Chris and Alex, thank you for review!

@sspitsyn
Copy link
Contributor Author

/backport jdk21

@openjdk
Copy link

openjdk bot commented Jun 21, 2023

@sspitsyn the backport was successfully created on the branch sspitsyn-backport-f91e9ba7 in my personal fork of openjdk/jdk21. To create a pull request with this backport targeting openjdk/jdk21:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit f91e9ba7 from the openjdk/jdk repository.

The commit being backported was authored by Serguei Spitsyn on 9 Jun 2023 and was reviewed by Chris Plummer and Alex Menkov.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21:

$ git fetch https://github.com/openjdk-bots/jdk21.git sspitsyn-backport-f91e9ba7:sspitsyn-backport-f91e9ba7
$ git checkout sspitsyn-backport-f91e9ba7
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21.git sspitsyn-backport-f91e9ba7

@sspitsyn sspitsyn changed the title 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING Backport f91e9ba Jun 21, 2023
@sspitsyn sspitsyn changed the title Backport f91e9ba 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING Jun 21, 2023
@sspitsyn sspitsyn deleted the br42 branch June 22, 2023 03:44
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