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

[David Holmes]

On Wed, 14 Dec 2022 16:48:35 GMT, Gerard Ziemski <gziemski 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` ?

Thanks for looking at this @gerard-ziemski . The Loom folk are quite adamant that `JavaThread::print_stack_on` always and only, prints the stack of the carrier/platform thread. That is why I previously added `JavaThread::print_vthread_stack_on` for virtual threads. It is up to the client code that wants to show the stack to decide what kind of stack they want to show - hence the logic based on `is_vthread_mounted()`. I previously added `JavaThread::print_jni_stack` with the same basic logic, but that will be further changed under another RFE I have, so can't be reused here. I could add yet-another `print_xxx_stack_on()` method to `JavaThread` where `xxx` means "print the virtual thread stack if it exists, else the carrier" - but I don't know what to use for `xxx` in that case.

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

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