[GR-44559] Finish ThreadMXBean implementation for Native Image. #8430
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
There was a problem hiding this comment.
Thanks for the PR, I added a few comments and questions.
I did not do a detailed review yet but I had a brief look at the HotSpot code for the same functionality and it uses a completely different approach (thread info is dumped during a VM operation). I think we will need something similar to avoid a large performance impact.
| @@ -91,6 +94,42 @@ public final class Target_java_lang_Thread { | |||
| @RecomputeFieldValue(kind = RecomputeFieldValue.Kind.NewInstance, declClass = ThreadData.class)// | |||
| UnacquiredThreadData threadData; | |||
|
|
|||
| @Inject // | |||
| @RecomputeFieldValue(kind = RecomputeFieldValue.Kind.Reset) // | |||
| long lastStartedWaiting = -1; | |||
There was a problem hiding this comment.
Please add a suffix to all fields that store a value in milli- or nanoseconds (e.g., lastStartedWaitingMs)
| @RecomputeFieldValue(kind = RecomputeFieldValue.Kind.Reset) // | ||
| long lastStartedWaiting = -1; | ||
|
|
||
| @Inject // |
There was a problem hiding this comment.
Please only inject a single new object field into the Thread object. This object field can then point to an object that holds all the other fields.
| @@ -1075,8 +1075,10 @@ public static int getThreadStatus(Thread thread) { | |||
| @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true) | |||
| public static void setThreadStatus(Thread thread, int threadStatus) { | |||
| assert !isVirtual(thread); | |||
There was a problem hiding this comment.
Does HotSpot only support contention monitoring for non-virtual threads?
|
|
||
| @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true) | ||
| private static void handleContentionWhenEnabled(Target_java_lang_Thread targetThread, int threadStatus) { | ||
| switch (threadStatus) { |
There was a problem hiding this comment.
Please also check for all other possible thread status values and add a default case. The new cases should either throw an error or do nothing.
|
|
||
| @Inject // | ||
| @RecomputeFieldValue(kind = RecomputeFieldValue.Kind.Reset) // | ||
| long timeBlocked = 0; |
There was a problem hiding this comment.
Is it possible to enable and disable contention monitoring at run-time? If so, what should happen with outdated values if contention monitoring gets enabled, disabled, and enabled again.
| * | ||
| * @return the owner thread | ||
| */ | ||
| @Alias |
There was a problem hiding this comment.
Methods that are only annotated with @Alias must not have an implementation.
| public record MonitorInfo(Object originalObject, int stacksize) { | ||
| } | ||
|
|
||
| public static void addThreadMonitor(Object originalObject) { |
There was a problem hiding this comment.
At the moment, you are calling this method unconditionally. This method has a large performance impact, so this definitely needs to be called conditionally.
| @Substitute() | ||
| protected void setExclusiveOwnerThread(Thread thread) { | ||
| if (thread == null && exclusiveOwnerThread != null) { | ||
| JavaThreads.JMXMonitoring.removeThreadLock(exclusiveOwnerThread, |
There was a problem hiding this comment.
How does HotSpot track contentions for ReentrantLocks?
| return state; | ||
| } | ||
|
|
||
| public static ThreadInfo getThreadInfo(Thread thread, int maxDepth, |
There was a problem hiding this comment.
What should this method return if the thread is no longer
a. alive
b. blocked
I think a lot of corner cases are missing here.
| Blocker blocker = getBlockerInfo(thread); | ||
| StackTraceElement[] stackTrace = getStackTrace(thread, maxDepth); | ||
| LockedMonitors lockedMonitors = getLockedMonitors(thread, stackTrace.length); | ||
| boolean inNative = stackTrace.length > 0 && stackTrace[0].isNativeMethod(); |
There was a problem hiding this comment.
This is not correct - you would need to access the VM-internal thread state of the isolate thread but this isn't accessible easily for threads other than the current thread.
|
@smthelusive could you please take a look at @christianhaeubl's comments? |
|
Hi @christianhaeubl, thank you for the review. |
See #7945. Closes #6101.