RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage
Chris Plummer
cjplummer at openjdk.org
Wed May 1 21:27:01 UTC 2024
On Wed, 1 May 2024 10:20:52 GMT, Serguei Spitsyn <sspitsyn at 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
More information about the serviceability-dev
mailing list