Add support for JFR monitor enter events #4651

roberttoyonaga · 2022-06-16T19:23:36Z

Added support for JFR jdk.JavaMonitorEnter events. These events provide information on when threads get blocked trying to acquire a lock.

graalvmbot · 2022-06-16T20:08:10Z

Hello Robert Toyonaga, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address rtoyonag -(at)- rtoyonag -(dot)- remote -(dot)- csb. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

Hello Robert Toyonaga, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address 35396843+roberttoyonaga -(at)- users -(dot)- noreply -(dot)- github -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

oraluben · 2022-06-17T01:53:33Z

within substrateVM it is not as easy to obtain addresses of Java objects.

Have you tried org.graalvm.compiler.word.Word#objectToTrackedPointer?

adinn · 2022-06-17T21:27:31Z

@christianhaeubl Could you please review this PR or suggest someone who could do so? Thanks!

christianhaeubl · 2022-06-20T08:27:48Z

...ratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/JavaMonitorEnterEvent.java

@@ -0,0 +1,48 @@
+package src.com.oracle.svm.core.src.com.oracle.svm.core.jfr.events;


Please add a copyright header.

christianhaeubl · 2022-06-20T08:28:37Z

...ratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/JavaMonitorEnterEvent.java

+public class JavaMonitorEnterEvent {
+
+    @Uninterruptible(reason = "Accesses a JFR buffer.")
+    public static void emit(Object obj, org.graalvm.nativeimage.IsolateThread previousOwner,long startTicks) {


Please run mx checkstyle and use the eclipse formatter.

christianhaeubl · 2022-06-20T09:11:46Z

...evm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java

@@ -255,8 +257,27 @@ private static void slowPathMonitorEnter(Object obj) {
    @RestrictHeapAccess(reason = NO_LONGER_UNINTERRUPTIBLE, access = Access.UNRESTRICTED)
    @Override
    public void monitorEnter(Object obj) {
+        long startTicks = com.oracle.svm.core.jfr.JfrTicks.elapsedTicks();


HotSpot only emits the event if there is a contention, i.e., no JFR event is emitted for the fast path. We need something similar.

I've now implemented this behavior by only emitting events if tryLock() fails to get the lock

This new approach doesn't work:

You only fill monitorOwners when a lock contention is happening. So, you will usually keep track of an outdated value and not of the latest previous owner. One possible solution would be to introduce a custom ReentrantLock subclass that keeps track of the previous owner in a field. Writing the JFR thread id to that field should be cheap enough so that you can do it in the fast path.

There is no guarantee that your call to lockObject.lock(); really runs into a contention. So, you will emit unnecessary JFR events from time to time. To solve that, you will probably need to add a method that is similar to lock() that returns if it was necessary to park the thread because of a contention.

yes I see! Thank you Christian. I have used the solution you suggested for 1.

For 2, wouldn't tryLock() returning false indicate that there is contention? Is the problem that the lock may be released in between the call to tryLock() and lock()? I think the Hotspot code makes one attempt to acquire the lock (+1 optional spin) and upon failure treats it as contention.

ad 2. you are right, I thought that HotSpot is more strict. So, the current solution is good enough.

christianhaeubl · 2022-06-20T09:18:23Z

...ratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/JavaMonitorEnterEvent.java

+            JfrNativeEventWriterData data = StackValue.get(JfrNativeEventWriterData.class);
+            JfrNativeEventWriterDataAccess.initializeThreadLocalNativeBuffer(data);
+
+            if (emit0(data, obj, previousOwner, startTicks, SubstrateJVM.get().isLarge(JfrEvent.JavaMonitorEnter)) == com.oracle.svm.core.jfr.JfrEventWriteStatus.RetryLarge) {


There shouldn't be any need for the retry mechanism as this event has a fixed size (it only contains primitive data). Search the code base for JfrNativeEventWriter.beginSmallEvent(...) to find examples where we emit JFR events that have a fixed size.

Please also use your Red Hat email address for all commits. Otherwise, we can't merge the changes to master easily.

ok I should be using my redhat email now. I got rid of the retry.

graalvmbot · 2022-06-20T14:08:11Z

Hello Robert Toyonaga, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address rtoyonag -(at)- redhat -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

graalvmbot · 2022-06-20T15:08:23Z

Robert Toyonaga has signed the Oracle Contributor Agreement (based on email address rtoyonag -(at)- redhat -(dot)- com) so can contribute to this repository.

christianhaeubl · 2022-06-22T07:22:36Z

...evm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java

@@ -255,8 +257,27 @@ private static void slowPathMonitorEnter(Object obj) {
    @RestrictHeapAccess(reason = NO_LONGER_UNINTERRUPTIBLE, access = Access.UNRESTRICTED)
    @Override
    public void monitorEnter(Object obj) {
+        long startTicks = com.oracle.svm.core.jfr.JfrTicks.elapsedTicks();


ad 2. you are right, I thought that HotSpot is more strict. So, the current solution is good enough.

christianhaeubl · 2022-06-22T07:52:14Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/OwnedReentrantLock.java

+import org.graalvm.nativeimage.CurrentIsolate;
+import com.oracle.svm.core.jfr.SubstrateJVM;
+
+public class OwnedReentrantLock extends ReentrantLock {


JavaMonitor might be a better name.

christianhaeubl · 2022-06-22T08:06:42Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/OwnedReentrantLock.java

+        currOwnerTid = SubstrateJVM.get().getThreadId(CurrentIsolate.getCurrentThread());
+    }
+
+    public boolean tryLockOwnedLock() {


You could rename this method to enter or monitorenter.
If you move the call to JavaMonitorEnterEvent.emit(...); into this method as well, then it should be sufficient to store the latest owner in a single field.

renamed and moved call to emit event

christianhaeubl · 2022-06-22T08:08:07Z

...evm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java

@@ -192,8 +193,10 @@ public class MultiThreadedMonitorSupport extends MonitorSupport {
     * Secondary storage for monitor slots. Synchronized to prevent concurrent access and
     * modification.
     */
-    private final Map<Object, ReentrantLock> additionalMonitors = new WeakIdentityHashMap<>();
+    private final Map<Object, OwnedReentrantLock> additionalMonitors = new WeakIdentityHashMap<>();
+    private final Map<Object, Long> monitorOwners = new WeakIdentityHashMap<>();


The fields monitorOwners and monitorOwnersLock are unused.

christianhaeubl

Thanks, I will try to merge this change in the next couple days.

roberttoyonaga marked this pull request as draft June 16, 2022 19:25

roberttoyonaga changed the title ~~[WIP] Add support for JFR monitor enter events~~ Add support for JFR monitor enter events Jun 17, 2022

roberttoyonaga marked this pull request as ready for review June 17, 2022 17:37

adinn requested a review from christianhaeubl June 17, 2022 21:26

christianhaeubl requested changes Jun 20, 2022

View reviewed changes

roberttoyonaga added 6 commits June 20, 2022 09:58

emit jdk.JavaMonitorEnter events

8200851

refactor

f16c401

add retry mechanism

040ca50

format event fields correctly

5db0d2b

add duration and start time

804754c

add address

3ee246d

roberttoyonaga force-pushed the addJavaMonitorEnter branch from 3316193 to 3ee246d Compare June 20, 2022 14:02

roberttoyonaga added 2 commits June 20, 2022 10:13

add copyright header and remove retry mechanism

f35b11a

try non-blocking try lock to see if lock is available first

a44e386

graalvmbot added the oca-signed label Jun 20, 2022

fix style

e928ecd

roberttoyonaga requested a review from christianhaeubl June 20, 2022 16:02

roberttoyonaga added 3 commits June 20, 2022 15:42

fix package

43f0d16

fix event type in get stacktrace

fe9cd54

make derived ReentrantLock class

29efe65

christianhaeubl reviewed Jun 22, 2022

View reviewed changes

rename and refactor

3e5753c

roberttoyonaga requested a review from christianhaeubl June 22, 2022 13:32

christianhaeubl approved these changes Jun 22, 2022

View reviewed changes

roberttoyonaga mentioned this pull request Jun 28, 2022

Add Support for JFR Java Monitor Wait and Thread Sleep events #4676

Closed

graalvmbot merged commit 3e5753c into oracle:master Jun 29, 2022

		@@ -0,0 +1,48 @@
		package src.com.oracle.svm.core.src.com.oracle.svm.core.jfr.events;

Add support for JFR monitor enter events #4651

Add support for JFR monitor enter events #4651

Uh oh!

Conversation

roberttoyonaga commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graalvmbot commented Jun 16, 2022

Uh oh!

oraluben commented Jun 17, 2022

Uh oh!

adinn commented Jun 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graalvmbot commented Jun 20, 2022

Uh oh!

graalvmbot commented Jun 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christianhaeubl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roberttoyonaga commented Jun 16, 2022 •

edited

Loading