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

Thread::getState is a scalability issue #17251

Open
DanHeidinga opened this issue Apr 24, 2023 · 5 comments · May be fixed by #21043
Open

Thread::getState is a scalability issue #17251

DanHeidinga opened this issue Apr 24, 2023 · 5 comments · May be fixed by #21043

Comments

@DanHeidinga
Copy link
Member

Java -version output

All versions

Summary of problem

The Netty project was using Thread::getState on a non-current thread. Due to needing to halt the thread for inspection, there was a scalability issue when using Netty on OpenJ9.

They've worked around it by calling Thread::isAlive instead for OpenJ9, but this kind of lingering scalability issue may affect others as well.

Original issue: netty/netty#13347 (comment)
Workaround: netty/netty#13357

@DanHeidinga
Copy link
Member Author

fyi @babsingh @gacholio

@babsingh
Copy link
Contributor

babsingh commented Apr 24, 2023

Potential solution: Add a field in j.l.Thread which will store the current state of the Java thread. It will be atomically updated as the Java thread goes through the various state transitions in native and Java code. With this approach, Thread::getState will require to atomically read the new field without the need to halt the thread for inspection.

@gacholio
Copy link
Contributor

gacholio commented May 4, 2023

We've talked about this for years - there's no need for an atomic update. Like so many APIs, the return value is momentary, likely to be different a few instructions later.

@ThanHenderson
Copy link
Contributor

In some stress tests I ran, the expense comes mostly from calling into native code and computing the state lazily; not so much from needing to halt the thread.

Adding a field (or using an existing unused field) to j.l.Thread then setting it on state transitions in our native code, like Babneet suggested, will alleviate this cost from the getState calls.

For the Java 19+ code, j.l.Thread has a nested FieldHolder class that holds a threadStatus field; we can use this to hold the thread state. We currently don't use this field for anything.

In pre-Java 19 j.l.Thread code, we can add this field in the same way as the other fields held in FieldHolder.

git blame shows that upstream uses this field similarly to what we've proposed here.

@gacholio
Copy link
Contributor

gacholio commented Oct 8, 2024

Updating thread state on transition instead of trying to derive it based on other information seems sensible to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants