RFR: 8298081: DiagnoseSyncOnValueBasedClasses doesn't report useful information for virtual threads

[Gerard Ziemski]

On Tue, 13 Dec 2022 06:24:34 GMT, David Holmes <dholmes at openjdk.org> wrote:

> The mechanism used by DiagnoseSyncOnValueBasedClasses to show where the sync occurs only reports platform/carrier thread stack details. That is no use if the code is being executed by a virtual thread. We can use the code being introduced by [JDK-8292674](https://bugs.openjdk.org/browse/JDK-8292674) for JNI error/warning reporting to show the virtual thread stack instead.
> 
> Before the fix the logging shows:
> 
> [0.173s][info][valuebasedclasses] Synchronizing on object 0x00000007ff880e68 of klass java.lang.Character
> [0.173s][info][valuebasedclasses] at jdk.internal.vm.Continuation.run([java.base at 21-internal](mailto:java.base at 21-internal)/Continuation.java:257)
> [0.173s][info][valuebasedclasses] at java.lang.VirtualThread.runContinuation([java.base at 21-internal](mailto:java.base at 21-internal)/VirtualThread.java:216)
> [0.173s][info][valuebasedclasses] at java.lang.VirtualThread$$Lambda$8/0x0000000801046f70.run([java.base at 21-internal](mailto:java.base at 21-internal)/Unknown Source)
> [0.173s][info][valuebasedclasses] at java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec([java.base at 21-internal](mailto:java.base at 21-internal)/ForkJoinTask.java:1423)
> [0.173s][info][valuebasedclasses] at java.util.concurrent.ForkJoinTask.doExec([java.base at 21-internal](mailto:java.base at 21-internal)/ForkJoinTask.java:387)
> [0.173s][info][valuebasedclasses] at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec([java.base at 21-internal](mailto:java.base at 21-internal)/ForkJoinPool.java:1312)
> [0.173s][info][valuebasedclasses] at java.util.concurrent.ForkJoinPool.scan([java.base at 21-internal](mailto:java.base at 21-internal)/ForkJoinPool.java:1843)
> [0.173s][info][valuebasedclasses] at java.util.concurrent.ForkJoinPool.runWorker([java.base at 21-internal](mailto:java.base at 21-internal)/ForkJoinPool.java:1808)
> [0.173s][info][valuebasedclasses] at java.util.concurrent.ForkJoinWorkerThread.run([java.base at 21-internal](mailto:java.base at 21-internal)/ForkJoinWorkerThread.java:188)
> 
> after the fix:
> 
> [0.177s][info][valuebasedclasses] Synchronizing on object 0x00000007ff880e68 of klass java.lang.Character
> [0.177s][info][valuebasedclasses] at SyncOnValueBasedClassTest$VTTest.lambda$main$0(SyncOnValueBasedClassTest.java:195)
> [0.177s][info][valuebasedclasses] - locked <0x00000007ff880e68> (a java.lang.Character)
> [0.177s][info][valuebasedclasses] at SyncOnValueBasedClassTest$VTTest$$Lambda$1/0x0000000801000c10.run(Unknown Source)
> [0.177s][info][valuebasedclasses] at java.lang.VirtualThread.runWith([java.base at 21-internal](mailto:java.base at 21-internal)/VirtualThread.java:335)
> [0.177s][info][valuebasedclasses] at java.lang.VirtualThread.run([java.base at 21-internal](mailto:java.base at 21-internal)/VirtualThread.java:305)
> [0.177s][info][valuebasedclasses] at java.lang.VirtualThread$VThreadContinuation.lambda$new$0([java.base at 21-internal](mailto:java.base at 21-internal)/VirtualThread.java:177)
> [0.177s][info][valuebasedclasses] at java.lang.VirtualThread$VThreadContinuation$$Lambda$7/0x0000000801046d48.run([java.base at 21-internal](mailto:java.base at 21-internal)/Unknown Source)
> [0.177s][info][valuebasedclasses] at jdk.internal.vm.Continuation.enter0([java.base at 21-internal](mailto:java.base at 21-internal)/Continuation.java:327)
> [0.177s][info][valuebasedclasses] at jdk.internal.vm.Continuation.enter([java.base at 21-internal](mailto:java.base at 21-internal)/Continuation.java:320)
> 
> 
> The existing test for `DiagnoseSyncOnValueBasedClasses` is adapted to provide a basic sanity test using virtual threads, which suffices to test the stack information. Unfortunately the `--enable-preview` has a viral impact.
> 
> Other testing: tiers 1-3 sanity check
> 
> Thanks.

Looks good to me.

I have one general question though, isn't there a way to do this in a more "object oriented" way, where we do not have to distinguish between 2 types of JavaThread, where one is `vthread` mounted and the other is not?

Maybe at least push the `is_vthread_mounted()` check inside `Thread.cpp` implementation of `print_stack_on()` itself, so that we don't need to call `is_vthread_mounted()` in `synchronizer.cpp` ?

-------------

PR: https://git.openjdk.org/jdk/pull/11641