Add some JFR event tests #4932

roberttoyonaga · 2022-09-12T19:11:50Z

Add some tests for recently added JFR events:

jdk.JavaMonitorEnter
jdk.JavaMonitorWait
jdk.ThreadSleep

Each test generates the desired events to test, creates a JFR snapshot, and inspects the snapshot. There are 4 jdk.JavaMonitorWait tests that each cover a different case (time out, interruption, notify all, notify).

christianhaeubl

Thanks for working on test cases for certain JFR events.

In a lot of your test cases, you try to use Thread.sleep() to ensure that multiple threads do something in a particular order. An approach like does not work reliably (especially if the machine has high load) and is therefore neither feasible for production code nor for test cases. You will need to switch to some kind of proper synchronization (e.g., ReentrantLock or a spin lock in case that you want to avoid that certain JFR events are emitted).

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/JfrTest.java

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorEnter.java

christianhaeubl · 2022-10-06T16:32:37Z

...ratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorWaitInterrupt.java

+        simpleNotifyName = tn.getName();
+
+        tw.start();
+        Thread.sleep(50);


This also won't work reliably.

christianhaeubl · 2022-10-06T16:40:37Z

...ratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorWaitInterrupt.java

+            if (!struct.<RecordedClass> getValue("monitorClass").getName().equals(Helper.class.getName())) {
+                continue;
+            }
+            assertTrue("Event is wrong duration.", isGreaterDuration(Duration.ofMillis(MILLIS), event.getDuration()));


This won't work reliably at the moment, see comments below.

christianhaeubl · 2022-10-06T16:47:22Z

...ratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorWaitNotifyAll.java

+
+        public synchronized void consume() throws InterruptedException {
+            // give the producers a headstart so they can start waiting
+            wait(MILLIS);


This won't work reliably.

christianhaeubl · 2022-10-07T11:48:56Z

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorWaitTimeout.java

+        notifierName = unheardNotifierThread.getName();
+
+        timeoutThread.start();
+        Thread.sleep(10);


This doesn't work reliably.

christianhaeubl · 2022-10-07T11:49:22Z

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestThreadSleep.java

+            if (!eventThread.equals(sleepingThreadName)) {
+                continue;
+            }
+            if (!isEqualDuration(event.getDuration(), Duration.ofMillis(MILLIS))) {


This won't work reliably.

roberttoyonaga · 2022-10-07T19:43:44Z

Thank you for the review Christian! I have updated the thread synchronization to be more reliable and removed the cases where sleeping is used to try to enforce ordering. Please let me know if there are still issues.

christianhaeubl

Thanks for the changes this already improves the situation. However, there is still one problematic pattern: querying Thread.getState() in a loop is unreliable because it doesn't tell the reason why a thread is blocked or waiting. So, the following can happen:

Thread A calls Thread.getState() on thread B and checks if thread B is blocked or waiting.
Thread B blocks or waits in JDK-internal code while the thread is being started (i.e. before it even reaches run()).
Thread A sees the changed thread status and assumes that thread B is blocked at the right position in run().

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorEnter.java

…-tests

roberttoyonaga · 2022-10-11T18:33:08Z

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorEnter.java

+                    Thread.sleep(10);
+                }
+                blockedAtCritical = true;
+                helper.doWork();


Is setting/checking of blockedAtCritical enough to guarantee the thread is blocking where we expect it to? I don't think it's possible to use the same pattern I've updated the monitor wait tests to. This is because monitor enter events don't seem to be emitted upon re-entry to synchronized blocks.

Yes, this should work. The name blockedAtCritical is a bit misleading though because the thread is not blocked yet (thread B just reached a point where thread A can use thread.getState() reliably to determine if thread B is blocked).

christianhaeubl · 2022-10-12T08:41:34Z

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorEnter.java

+                    Thread.sleep(10);
+                }
+                blockedAtCritical = true;
+                helper.doWork();


Yes, this should work. The name blockedAtCritical is a bit misleading though because the thread is not blocked yet (thread B just reached a point where thread A can use thread.getState() reliably to determine if thread B is blocked).

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorEnter.java

christianhaeubl

Thanks, your PR will be merged to master in the next couple days.

roberttoyonaga added 4 commits September 9, 2022 16:41

add JFR event unit tests for thread sleep and monitor enter

481f81c

add java monitor wait

d0f03cf

make tests more robust

98e3ac9

add monitor wait interrupt test. checkstyle, format

06a3c9f

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 12, 2022

roberttoyonaga added 2 commits September 12, 2022 15:50

fix hiding field and exceptions

c7b5486

remove unused try catch blocks

5bdeff6

fniephaus requested a review from christianwimmer September 15, 2022 08:51

fniephaus assigned roberttoyonaga Sep 15, 2022

fniephaus added this to the 23.0 milestone Sep 15, 2022

christianwimmer requested review from christianhaeubl and peter-hofer and removed request for christianwimmer September 22, 2022 14:41

christianhaeubl requested changes Oct 7, 2022

View reviewed changes

roberttoyonaga added 2 commits October 7, 2022 14:11

use spinlocks for synchronization. Refactor.

727adae

filter in getEvents and checkstyle

ed0f1ff

fixes for gate check

5b37c49

roberttoyonaga requested review from christianhaeubl and removed request for peter-hofer October 7, 2022 19:48

more fixes for gate

4e26325

christianhaeubl requested changes Oct 10, 2022

View reviewed changes

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/jfr/TestJavaMonitorEnter.java Outdated Show resolved Hide resolved

roberttoyonaga added 5 commits October 11, 2022 10:43

set flag before blocking/waiting

417d9f7

update wait tests

2902c0b

Merge branch 'master' of github.com:roberttoyonaga/graal into add-jfr…

156cee1

…-tests

fix conflicts

8a3d0ee

fix error and checkstyle

4955e21

roberttoyonaga commented Oct 11, 2022

View reviewed changes

roberttoyonaga requested a review from christianhaeubl October 11, 2022 18:43

update visibility

cd40dc2

christianhaeubl reviewed Oct 12, 2022

View reviewed changes

rename variable. Have child threads start eachother

656ddfd

roberttoyonaga requested a review from christianhaeubl October 12, 2022 20:12

christianhaeubl approved these changes Oct 14, 2022

View reviewed changes

graalvmbot merged commit c14576f into oracle:master Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some JFR event tests #4932

Add some JFR event tests #4932

roberttoyonaga commented Sep 12, 2022 •

edited

Loading

christianhaeubl left a comment

christianhaeubl Oct 6, 2022

christianhaeubl Oct 6, 2022

christianhaeubl Oct 6, 2022

christianhaeubl Oct 7, 2022

christianhaeubl Oct 7, 2022

roberttoyonaga commented Oct 7, 2022

christianhaeubl left a comment

roberttoyonaga Oct 11, 2022 •

edited

Loading

christianhaeubl Oct 12, 2022

christianhaeubl Oct 12, 2022

christianhaeubl left a comment

Add some JFR event tests #4932

Add some JFR event tests #4932

Conversation

roberttoyonaga commented Sep 12, 2022 • edited Loading

christianhaeubl left a comment

Choose a reason for hiding this comment

christianhaeubl Oct 6, 2022

Choose a reason for hiding this comment

christianhaeubl Oct 6, 2022

Choose a reason for hiding this comment

christianhaeubl Oct 6, 2022

Choose a reason for hiding this comment

christianhaeubl Oct 7, 2022

Choose a reason for hiding this comment

christianhaeubl Oct 7, 2022

Choose a reason for hiding this comment

roberttoyonaga commented Oct 7, 2022

christianhaeubl left a comment

Choose a reason for hiding this comment

roberttoyonaga Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

christianhaeubl Oct 12, 2022

Choose a reason for hiding this comment

christianhaeubl Oct 12, 2022

Choose a reason for hiding this comment

christianhaeubl left a comment

Choose a reason for hiding this comment

roberttoyonaga commented Sep 12, 2022 •

edited

Loading

roberttoyonaga Oct 11, 2022 •

edited

Loading