-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Implementation of Safepoint events #4330
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
Conversation
ee3f55d
to
360cbf0
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.
Thanks again for contributing, I added a small number of comments.
public class TimedEvent { | ||
@Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true) | ||
public static long getTicks() { | ||
return HasJfrSupport.get() ? JfrTicks.elapsedTicks() : 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.
Seems that this pattern will be pretty common.
I think it makes sense to move the HasJfrSupport.get()
check directly into JfrTicks.elapsedTicks()
(JfrTicks
is a helper class anyways). Then, it is possible to unconditionally call JfrTicks.elapsedTicks()
, regardless if the JFR support is present in the image or not.
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.
Okay.
|
||
// Walk the threads list and ask each thread (except myself) to come to a safepoint. | ||
for (IsolateThread vmThread = VMThreads.firstThread(); vmThread.isNonNull(); vmThread = VMThreads.nextThread(vmThread)) { | ||
if (isMyself(vmThread)) { | ||
continue; | ||
} | ||
numOfThreads++; |
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.
Shouldn't this count also include the current thread? This code is always executed by the VM operation thread but the VM operation thread is a normal Java thread in Native Image. We support two different execution modes (see VMOperationControl.useDedicatedVMOperationThread()
):
a. Without a dedicated VM operation thread, any normal Java thread can be temporarily promoted to the VM operation thread.
b. If a dedicated VM operation thread should be used, then a dedicated Java thread is started early on and only that thread may execute VM operations.
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 it means how many threads were brought to the safepoint. In hotspot, it does not count vmThread.
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.
HotSpot reports the number of Java threads, so I think we should the same.
However, in our case that includes the VM operation thread because it is a Java thread as well. This is a major difference to HotSpot anyways and affects all JFR areas so I think it is best to consistently report the VM operation thread as a normal Java thread (e.g., the VM operation thread can allocate Java heap memory, so it will also show up in all allocation-related JFR events).
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.
Okay, fixed
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 PR will also be merged to Graal master in the next couple days.
31b85a2
to
74510f8
Compare
Add implementation of Safepoint event that provides safepoint latency information.
e.g.