-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[GR-44559] Finish ThreadMXBean implementation for Native Image. #7945
Conversation
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
565eed1
to
8da4ed7
Compare
Thank you for signing the OCA. |
Haven't look at this in detail yet, but full support for ThreadMXBean would be very nice to have! You may need to run |
8da4ed7
to
21d598c
Compare
Thank you for the tip @roberttoyonaga! Done 👍 |
21d598c
to
984c3c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @smthelusive ! It would be very nice to have this ThreadMXBean support. I left some comments/concerns below.
public static class ThreadInfoConstructionUtils { | ||
|
||
private static StackTraceElement[] getStackTrace(Thread thread, int maxDepth) { | ||
return maxDepth == -1 || maxDepth >= thread.getStackTrace().length ? thread.getStackTrace() : Arrays.copyOfRange(thread.getStackTrace(), 0, maxDepth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the thread handling the ThreadMXBean call is likely not the thread we're getting the stacktrace for, calling thread.getStackTrace()
will result in a safepoint operation in order to do the stack walk. So to reduce pause time, I think we should avoid calling thread.getStackTrace()
twice in this method.
private static int getThreadState(Thread thread) { | ||
int state = PlatformThreads.getThreadStatus(thread); | ||
boolean inNative = VMThreads.StatusSupport | ||
.getStatusVolatile(PlatformThreads.getIsolateThreadUnsafe(thread)) == VMThreads.StatusSupport.STATUS_IN_NATIVE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's safe here to use the isolate thread (which may not be the current thread) without first holding the VMThreads#THREAD_MUTEX
or being in a VM operation. This is because getStatusVolatile
reads the thread local StatusSupport#statusTL
. I think it's possible for the isolate thread we're dealing with to go away unexpectedly, which would result in illegally accessing that thread local. @christianhaeubl can you confirm this?
@@ -248,6 +248,9 @@ public void monitorEnter(Object obj, MonitorInflationCause cause) { | |||
if (monitor == null) { // successful | |||
JavaMonitorInflateEvent.emit(obj, startTicks, MonitorInflationCause.MONITOR_ENTER); | |||
newMonitor.latestJfrTid = current; | |||
JavaThreads.JMXMonitoring.addThreadMonitor(newMonitor); | |||
newMonitor.blockedObject = obj; | |||
newMonitor.depth = Thread.currentThread().getStackTrace().length; | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the docs here it seems like blockObject
and ownerThread
should only be set when the thread is actually blocked and unable to enter the critical section. So, I don't think the instrumentation here is correct since this is the monitor enter fast path.
Additionally, the instrumentation needs to be added for the slow path as well (when there actually is contention).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @roberttoyonaga! Thank you for reviewing my PR.
Let me clarify on this section a bit. I'm not actually setting a thread blocker here. It's been handled before me, and in the end I'm getting the blocker by reading Thread.parkBlocker
field.
The problem is that when the thread is blocked upon a monitor, that field will contain an instance of JavaMonitor
. JavaMonitor
represents the monitor, but does not save any information about the original Object
with that blocked monitor, so I need to save it when the new JavaMonitor
is created.
I see that I did miss some things:
- I do it in fast path where we're creating a new
JavaMonitor
, however I'm not handling the use-case when we're creating new monitor foradditionalMonitors
, so I'll make a change; - No need to set the
blockedObject
tonull
onmonitorExit
later in this class; - I need to update the depth when re-entering the same monitor, and also on
monitorExit
whenmonitor.acquisitions > 1
.
Please let me know if that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see. So you only ever need to write blockedObject
upon monitor inflation. And you're relying on LockSupport.getBlocker
to get the status of the thread before reading that field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly 👍
*/ | ||
@SuppressWarnings({"unused"}) | ||
@TargetClass(className = "java.util.concurrent.locks.AbstractOwnableSynchronizer", onlyWith = JmxIncluded.class) | ||
@Substitute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick. I think you don't really have to substitute the whole class here. You could try just substituting setExclusiveOwnerThread
(you may need to alias the exclusiveOwnerThread
field)
@@ -306,6 +309,11 @@ public void monitorExit(Object obj, MonitorInflationCause cause) { | |||
monitor = getOrCreateMonitor(obj, cause); | |||
} | |||
monitor.monitorExit(); | |||
if (monitor.acquisitions == 1) { | |||
monitor.blockedObject = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the thread still qualify as "blocked" when it is no longer waiting and enters the critical section? If not, then maybe this deaccounting should be moved to the end of JavaMonitor#monitorEnter
?
Object[] monitorObjects = new Object[monitors != null ? monitors.size() : 0]; | ||
int[] monitorDepths = new int[monitorObjects.length]; | ||
for (int i = 0; i < monitorObjects.length; i++) { | ||
JavaMonitor javaMonitor = monitors.iterator().next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it thread-safe to be iterating the monitors without synchronization? The target thread can be running and its set of monitors may change while we are in this method.
For example, what if monitors
has size 10 at line 95, then by the time we reach line 97 monitors
has size 5. Then monitorObjects.length != monitors.size()
.
return new Blocker(blocker, null); | ||
} | ||
|
||
private static Object[] getLockedSynchronizers(Thread thread) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concern in this method as in getLockedMonitors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR, well done @smthelusive!
Since threading is rather important, the PR will need to be reviewed carefully. Please feel free to reach out if you have any questions or in case the PR (or you) get stuck. 😉
I'm going to run our internal gates on this. I will report any issues here...
@@ -0,0 +1,43 @@ | |||
/* | |||
* Copyright (c) 2017, 2023, Oracle and/or its affiliates. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update all years in copyright headers in files that you are adding. They should refer to the actual year(s). Since it's unlikely that this will still be merged this year (we are getting close to the Xmas break), I think you can just use this already:
* Copyright (c) 2017, 2023, Oracle and/or its affiliates. All rights reserved. | |
* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved. |
private static final HashSet<Long> deadlocked = new HashSet<>(); | ||
private static final HashSet<Long> chain = new HashSet<>(); | ||
private static ThreadInfo[] allThreadInfos = new ThreadInfo[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this global state really needed? Could you allocate the objects in findDeadlockedThreads
and pass them to the other private helpers instead?
} | ||
|
||
@Override | ||
public ThreadInfo[] getThreadInfo(long[] ids) { | ||
return new ThreadInfo[0]; | ||
return (ThreadInfo[]) Arrays.stream(ids).mapToObj(this::getThreadInfo).toArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you re-use getThreadInfo(long[] ids, int maxDepth)
here?
return (ThreadInfo[]) Arrays.stream(ids).mapToObj(this::getThreadInfo).toArray(); | |
return getThreadInfo(ids, -1); |
Here's a
|
984c3c2
to
e7cd052
Compare
Hi @fniephaus and @roberttoyonaga. Thank you both for the review! I've fixed the points that you mentioned and reworked some parts. Here is what changed:
Let me know if you have more suggestions, I will do my best to fix it. :) |
e7cd052
to
0177e46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy new year @smthelusive! Thank you for the changes. You've addressed most of my previous questions/concerns. I have left a few more below.
* synchronized block occurred. | ||
*/ | ||
int stackSizeWithoutInternalLogic = -1; | ||
if (MonitorInflationCause.MONITOR_ENTER.equals(cause)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes in openJDK, VM level frames get skipped, but not in SubstrateVM. JFR has the same issue. I don't know if hard-coding the frame skip-count is the best solution since it will require remembering to do maintenance whenever the monitor enter / stackwalking implementation is changed. Personally, I think checking the module/package names is better (but others may disagree).
I think we should at least add a comment to explicitly specify that the hardcoded value needs to be updated whenever the implementation changes.
Also, what about other inflation causes? Isn't it always the case that we'll need to remove some internal frames from the stacktrace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a change to check the module names instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This seems better now.
if (monitors != null) { | ||
synchronized (monitors) { | ||
Object[] monitorObjects = monitors.stream().map(JMXMonitoring.MonitorInfo::originalObject).toArray(); | ||
int[] monitorDepths = monitors.stream().mapToInt(monitorInfo -> stacktraceLength < 0 ? -1 : stacktraceLength - monitorInfo.stacksize()).toArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you do the subtraction stacktraceLength - monitorInfo.stacksize()
? Shouldn't the stackdepth only be monitorInfo.stacksize()
(since you already substracted the internal frames in addThreadMonitor
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @roberttoyonaga, happy new year! Thank you for your review. I will look into the comments and make the changes.
For this one, I made a drawing to illustrate what I'm doing here:
private static LockedMonitors getLockedMonitors(Thread thread, int stacktraceLength) { | ||
ArrayDeque<JMXMonitoring.MonitorInfo> monitors = JMXMonitoring.getThreadMonitors(thread); | ||
if (monitors != null) { | ||
synchronized (monitors) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is still a race here. I don't think it's enough to only synchronize here when we're reading the monitors. We also need to synchronize when we modify the stack of monitors in removeThreadMonitor
and addThreadMonitor
. For example, if monitors
is modified in between when monitorObjects
and monitorDepths
is assigned, monitorObjects
and monitorDepths
won't be the same size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @roberttoyonaga, I see.
So, only the thread that owns the monitors is modifying the monitors collection. The other thread is reading it here. I have made a change to get an unmodifiable copy of the monitors list, so then I can use it without worrying about changes in the original monitors collection. I think I shouldn't block the writes when I'm reading the collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I think this idea of getting a "snapshot" of the list is good. However, there is still the possibility that the list is modified while List.copyOf()
is in-progress (race during construction). I'm not sure how bad this could be because it depends on the implementation of List.copyOf()
, ArrayDeque.push
ArrayDeque.pop
private static Object[] getLockedSynchronizers(Thread thread) { | ||
EconomicSet<AbstractOwnableSynchronizer> locks = JMXMonitoring.getThreadLocks(thread); | ||
if (locks != null) { | ||
synchronized (locks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is still a race here. I don't think it's enough to only synchronize here when we're reading the locks. We also need to synchronize when we modify the set of locks . For example, if the set of locks is modified by the target thread in between when we assign lockObjects
a size and when we finish iterating the set, then we may try to access an index that doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @roberttoyonaga, indeed there was such possibility.
So, only one thread is adding or removing locks - it's the thread that owns them. Another thread is reading these values for reporting here. I did a small change here to use an iterator over the locks. This way I'm sure that any modifications of the underlying collection won't affect me when I'm reading the collection (and I think I shouldn't block the writes while reading it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better than before, but doesn't prevent the race. While iterating the list here, the owning thread may concurrently remove items from the list. Will this cause problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @roberttoyonaga,
when the values get removed, they are set to null (in an array), and on iterating, the null values are skipped. However, there might be a problem when the size shrinks, if that happens in between hasNext()
and next()
calls.
I've made a change. It's really convenient to continue using the EconomicSet
since I can provide it with equivalence strategy that uses System.identityHashCode
. So I decided to go for external synchronization. I'm not using the ReadWriteLock
here because that adds an extra lock that the thread obtains while it's already busy adding a lock, so it will loop trying to add more lock entries. So the monitor it is.
I've also made a change for the monitors collection, where I'm using now ConcurrentLinkedDeque
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the monitor it is
I think your new approach has solved the concurrency problem here. However, I think it's possible to easily optimize this with more fine-grained locking. You could maybe synchronize on the lock collection itself (Target_java_lang_Thread#locks
) so multiple threads can work simultaneously on different collections. Is there a reason you are using a global lock instead?
I've also made a change for the monitors collection, where I'm using now ConcurrentLinkedDeque.
What is the purpose of this change? Changing to a ConcurrentLinkedDeque
does not make the construction of the list copy atomic (return List.copyOf(targetThread.monitors);
). So I still have the same comment as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I shouldn't be using the global lock. Fixed that.
Regarding the ConcurrentLinkedDeque
, I should have used the Iterator that it provides instead of List.copyOf
. According to the documentation, this Iterator is weakly consistent:
- they may proceed concurrently with other operations
- they will never throw
ConcurrentModificationException
- they are guaranteed to traverse elements as they existed upon construction exactly once, and may (but are not guaranteed to) reflect any modifications subsequent to construction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Both concerns are resolved now.
75984b5
to
5397a4d
Compare
private static void setThreadBlockedToEnterLock(Target_java_lang_Thread thread) { | ||
if (thread.lastStartedBlocked == NO_VALUE) { | ||
thread.lastStartedBlocked = System.currentTimeMillis(); | ||
thread.blockedCount++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we guaranteed to always be dealing with the current thread? If so, maybe it's worth adding an assertion here.
Otherwise, is it safe to be un-atomically modifying thread.lastStartedBlocked
, thread.blockedCount
, thread.waitedCount
, etc ? Or do we require synchronization here as well? Or do you assume it's safe since we just started waiting/blocking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the thread states that I'm interested in, it's modified by the current thread only.
However, there are use-cases such as when a new thread is initialized, then its state can be set by its parent just before the new thread is assigned as current. In this case, the thread state will be set to NEW
, and I'm not updating anything for this state.
d50524d
to
f447bc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this @smthelusive! These changes look good to me now.
It would be nice to also have some tests for this MXBean in the future as well.
f447bc2
to
7e5ae47
Compare
* the locks per thread when JMX support is enabled. | ||
*/ | ||
@SuppressWarnings({"unused"}) | ||
@TargetClass(value = java.util.concurrent.locks.AbstractOwnableSynchronizer.class, onlyWith = JmxIncluded.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last minor comment. I've poorly named this:JmxIncluded
is based on whether remote JMX support is included. I think we want to have this substitution even if remote JMX is excluded because we can still use the MBeanServer locally.
7e5ae47
to
a6ed7e9
Compare
Hi @fniephaus and @christianhaeubl! |
Hey @smthelusive, thanks for the update! I'm going to run the public and internal gates on your PR again. Hopefully, we soon have enough time to review your PR as well. We are currently busy integrating other contributions from @roberttoyonaga and others. |
This PR unfortunately got mirrored to #8430, sorry about that. I am adding fixes for the style gate so that other gates can run as well. |
Looks like our internal gate has found a problem with the PR:
This suggests that the implementation may not be working correctly with virtual threads. Could you take a look, please? |
Hi all, thank you for your reviews. |
The functionality of
ThreadMXBean
is implemented and working within native image GR-44559Testing
Build native image with:
--enable-monitoring=jmxserver,jvmstat
Run it with:
-Dcom.sun.management.jmxremote.authenticate=false -Dcom.sun.management.jmxremote.port=9996 -Dcom.sun.management.jmxremote.ssl=false -Dcom.sun.management.enableThreadContentionMonitoring=true
Tools like VisualVM show the Thread monitoring info:
Example result of
dumpAllThreads
method call:Result of
getThreadInfo
by Thread id (the locks and monitors shouldn't be included):Deadlock detection returns an array of ids of deadlocked threads:
Closes #6101