Skip to content

Conversation

zhengyu123
Copy link
Contributor

@zhengyu123 zhengyu123 commented Feb 15, 2022

Add implementation of Safepoint event that provides safepoint latency information.

e.g.

jdk.SafepointBegin {
  startTime = 15:43:26.488
  duration = 4.315 us
  safepointId = 2
  totalThreadCount = 5
  jniCriticalThreadCount = 0
  eventThread = "main" (javaThreadId = 1)
}

jdk.SafepointEnd {
  startTime = 15:43:26.490
  duration = 0.318 us
  safepointId = 2
  eventThread = "main" (javaThreadId = 1)
}


@zhengyu123 zhengyu123 marked this pull request as ready for review February 15, 2022 20:48
Copy link
Member

@christianhaeubl christianhaeubl left a 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;
Copy link
Member

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.

Copy link
Contributor Author

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++;
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, fixed

Copy link
Member

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.

@graalvmbot graalvmbot merged commit bc31900 into oracle:master Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants