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

Patch MXBean to support virtual threads #15956

Merged
merged 1 commit into from
Oct 22, 2022

Conversation

thallium
Copy link
Contributor

@thallium thallium commented Sep 23, 2022

Use carrierThreadObject instead of threadObject for Java 19.

For thread with a virtual thread mount that is waiting on a lock,
we should return the virtual thread object as lock object/owner.

Fixes #15941

@thallium
Copy link
Contributor Author

@fengxue-IS FYI

@fengxue-IS fengxue-IS added the project:loom Used to track Project Loom related work label Sep 27, 2022
@thallium thallium changed the title [WIP] Patch MXBean to support virtual threads Patch MXBean to support virtual threads Sep 27, 2022
Copy link
Contributor

@fengxue-IS fengxue-IS left a comment

Choose a reason for hiding this comment

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

minor formating suggestion, rest lgtm.

@thallium as the change in thrinfo.c may impact other code that makes use of it to retrieve lock info, can you verify this change doesn't break any of our existing tests (sanity.functional / sanity.openjdk / extended.functional + jvmti test from the serviceability suite)

runtime/util/thrinfo.c Outdated Show resolved Hide resolved
@thallium
Copy link
Contributor Author

sanity.functional and extended.functional test passed, sanity.openjdk failed to start, not sure if it's a configuration problem
https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/14322/

Copy link
Contributor

@fengxue-IS fengxue-IS left a comment

Choose a reason for hiding this comment

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

LGTM

Per offline discussion, this change in thrinfo didn't impact testing that have been fixed in #15914 (comment)

@babsingh Can you take a look please.

runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Show resolved Hide resolved
runtime/util/thrinfo.c Outdated Show resolved Hide resolved
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

Accidentally approved. Wanted to request changes.

runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/util/thrinfo.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Show resolved Hide resolved
runtime/util/thrinfo.c Outdated Show resolved Hide resolved
runtime/util/thrinfo.c Outdated Show resolved Hide resolved
runtime/util/thrinfo.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Show resolved Hide resolved
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

final feedback. will launch builds after this is addressed.

runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtthread.c Outdated Show resolved Hide resolved
runtime/util/thrinfo.c Outdated Show resolved Hide resolved
runtime/util/thrinfo.c Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

Also, full-stops are missing in the commit message. See the updated PR description.

Use carrierThreadObject instead of threadObject for Java 19.

For thread with a virtual thread mount that is waiting on a lock,
we should return the virtualthreadobject as lock object/owner.

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
@babsingh
Copy link
Contributor

jenkins test sanity amac,win jdk19,jdk11

@babsingh babsingh merged commit 5803916 into eclipse-openj9:master Oct 22, 2022
babsingh added a commit to babsingh/openj9 that referenced this pull request Oct 24, 2022
Fixes the typo introduced in eclipse-openj9#15956. targetThread should be used instead
of currentThread for retrieving the threadObject.

Related: eclipse-openj9#14538

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project:loom Used to track Project Loom related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loom: test failure in HoldsLock
4 participants