On Wed, 1 May 2024 10:20:52 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> The fix is to degrade virtual threads support in the JVM TI > `GetObjectMonitorUsage` function so that it is specified to only return an > owner when the owner is a platform thread. Also, virtual threads are not > listed in the both `waiters` and `notify_waiters` lists returned in the > `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI > functions and events for virtual threads, we missed this one. > > The main motivation for degrading it now is that the object monitor > implementation is being updated to allow virtual threads unmount while owning > monitors. It would add overhead to record monitor usage when > freezing/unmount, overhead that couldn't be tied to a JVMTI capability as the > capability can be enabled at any time. > > `GetObjectMonitorUsage` was broken for 20+ years > ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports > so it seems unlikely that the function is widely used. Degrading it to only > return an owner when the owner is a platform thread has no compatibility > impact for tooling that uses it in conjunction with `HotSpot` thread dumps or > `ThreadMXBean`. > > One other point about `GetObjectMonitorUsage` is that it pre-dates > j.u.concurrent in Java 5 so it can't be used to get a full picture of the > lock usage in a program. > > The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the > JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` > methods are updated to match the JVM TI spec. > > Also, please, review the related CSR and Release Note: > - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade > virtual thread support for GetObjectMonitorUsage > - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: > degrade virtual thread support for GetObjectMonitorUsage > > Testing: > - tested impacted and updated tests locally > - tested with mach5 tiers 1-6 I've only looked at specs and tests so far. Still need to review the JVMTI code changes. I looked at the CSR too, but thought it best to just comment on the spec changes here. src/hotspot/share/prims/jvmti.xml line 8259: > 8257: <jint/> > 8258: <description> > 8259: The number of times the owning platform thread has entered > the monitor "the owning platform thread" doesn't really make sense if the monitor is owned by a virtual thread. You might want structure it more like the "owner" description above: The number of times the platform thread owning this monitor has has entered it, or <code>0</code> if owned by a virtual thread or not owned src/hotspot/share/prims/jvmti.xml line 8266: > 8264: <description> > 8265: The number of platform threads waiting to own this monitor, > 8266: or <code>0</code> if the monitor is owned by a virtual > thread or not owned Be consistent with above descriptions. They don't say "if the monitor is owned by". They say "if owned by". src/hotspot/share/prims/jvmti.xml line 8279: > 8277: <description> > 8278: The number of platform threads waiting to be notified by > this monitor, > 8279: or <code>0</code> if the monitor is owned by a virtual > thread or not owned Same consistency issue as with `waiter_count` src/java.se/share/data/jdwp/jdwp.spec line 1620: > 1618: ) > 1619: (Reply > 1620: (threadObject owner "The platform thread owning this > monitor, or <code>nullptr</code> " I don't think we should be introducing `<code>nullptr</code>` for just this one location. Please stick with `null` for now. src/java.se/share/data/jdwp/jdwp.spec line 1621: > 1619: (Reply > 1620: (threadObject owner "The platform thread owning this > monitor, or <code>nullptr</code> " > 1621: "if owned` by a virtual thread or not > owned.") You have a dangling back quote after "owned". This is showing up in the CSR too. src/java.se/share/data/jdwp/jdwp.spec line 1622: > 1620: (threadObject owner "The platform thread owning this > monitor, or <code>nullptr</code> " > 1621: "if owned` by a virtual thread or not > owned.") > 1622: (int entryCount "The number of times the owning platform > thread has entered the monitor.") See the comment I left for the JVMTI spec. We should be more complete in the explanation here, explaining how it is 0 for virtual threads. src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java line 348: > 346: /** > 347: * Returns a List containing a {@link ThreadReference} for > 348: * each platform thread currently waiting for this object's monitor. You need to add "platform" a little below in the `@return` section. src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java line 369: > 367: > 368: /** > 369: * Returns an {@link ThreadReference} for the platform thread, if > any, Pre-existing issue: It should be "a" not "an", but then in the `@return` section we are using "the", so maybe we should use similar wording here: `...the {@link ThreadReference} of the platform thread...` test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java line 257: > 255: // Correct the expected values for the virtual thread case. > 256: int expEnteringCount = isVirtual ? 0 : > NUMBER_OF_ENTERING_THREADS; > 257: int expWaitingCount = isVirtual ? 0 : > NUMBER_OF_WAITING_THREADS; There are comments below that still reference NUMBER_OF_ENTERING_THREADS and NUMBER_OF_WAITING_THREADS. test/hotspot/jtreg/vmTestbase/nsk/jdi/ObjectReference/waitingThreads/waitingthreads002.java line 167: > 165: try { > 166: List waitingThreads = objRef.waitingThreads(); > 167: if (waitingThreads.size() != expWaitingCount) { I don't see the need for the expWaitingCount bookkeeping. Can't we just verify that size() is zero if we are using virtual threads? I guess maybe the reason you took this approach is because you don't know if the threads are going to be virtual or not until you check them. There is a way to find out, but it's not that pretty either: static final boolean vthreadMode = "Virtual".equals(System.getProperty("test.thread.factory")); test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001.java line 65: > 63: } > 64: // Virtual threads are not supported by the > GetObjectMonitorUsage. Correct > 65: // the expected values if the test is executed with > MainWrapper=virtual. "MainWrapper" is not the proper terminology any more. It's "Test Thread Factory" (JTREG_TEST_THREAD_FACTORY=Virtual). test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001.java line 158: > 156: public void run() { > 157: // Virtual threads are not supported by the > GetObjectMonitorUsage. Correct > 158: // the expected values if the test is executed with > MainWrapper=virtual. "MainWrapper" again. test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage004.java line 64: > 62: synchronized (lockCheck) { > 63: // Virtual threads are not supported by the > GetObjectMonitorUsage. Correct > 64: // the expected values if the test is executed with > MainWrapper=virtual. "MainWrappe" again. ------------- PR Review: https://git.openjdk.org/jdk/pull/19030#pullrequestreview-2034390826 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586784250 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586784280 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586792380 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586800777 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586802318 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586803324 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586806802 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586809854 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586833617 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586821719 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586824426 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586827714 PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1586829010
