On Fri, 1 Mar 2024 11:50:05 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> rename after merge: jvmti_common.h to jvmti_common.hpp
Thanks Serguei, this is generally looking good, but I have one main query with
the logic, a number of suggestions regarding comments, and a few issues in the
new test code.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1489:
> 1487: assert(mon != nullptr, "must have monitor");
> 1488: // this object has a heavyweight monitor
> 1489: nWant = mon->contentions(); // # of threads contending for monitor
Please clarity comment as
// # of threads contending for monitor entry, but not -rentry
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1490:
> 1488: // this object has a heavyweight monitor
> 1489: nWant = mon->contentions(); // # of threads contending for monitor
> 1490: nWait = mon->waiters(); // # of threads in Object.wait()
Please clarify the comment as
// # of threads waiting for notification, or to re-enter monitor, in
Object.wait()
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1491:
> 1489: nWant = mon->contentions(); // # of threads contending for monitor
> 1490: nWait = mon->waiters(); // # of threads in Object.wait()
> 1491: wantList = Threads::get_pending_threads(tlh.list(), nWant + nWait,
> (address)mon);
Please add a comment
// Get the actual set of threads trying to enter, or re-enter, the monitor.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1505:
> 1503: // the monitor threads after being notified. Here we are
> correcting the actual
> 1504: // number of the waiting threads by excluding those re-entering
> the monitor.
> 1505: nWait = i;
Sorry I can't follow the logic here. Why is the whole block not simply:
if (mon != nullptry) {
// The nWait count may include threads trying to re-enter the monitor, so
// walk the actual set of waiters to count those actually waiting.
int count = 0;
for (ObjectMonitor * waiter = mon->first_waiter(); waiter != nullptr; waiter
= mon->next_waiter(waiter)) {
count++;
}
}
ret.notify_waiter_count = count;
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1552:
> 1550: // If the thread was found on the ObjectWaiter list, then
> 1551: // it has not been notified. This thread can't change the
> 1552: // state of the monitor so it doesn't need to be suspended.
Not sure why suspension is being mentioned here.
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
line 31:
> 29: * DESCRIPTION
> 30: * The test checks if the JVMTI function GetObjectMonitorUsage returns
> 31: * the expected values for the owner, entry_count, water_count
s/water_count/waiter_count/
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
line 40:
> 38: * - owned object with N waitings to be notified
> 39: * - owned object with N waitings to enter, from 0 to N waitings to
> re-enter,
> 40: * from N to 0 waitings to be notified
"waitings" is not grammatically valid. Suggestion: s/waitings/threads waiting/
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
line 42:
> 40: * from N to 0 waitings to be notified
> 41: * - all the above scenarios are executed with platform and virtual
> threads
> 42: * @requires vm.continuations
Do we really require this?
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
line 132:
> 130: // count of threads waiting to re-enter: 0
> 131: // count of threads waiting to be notified:
> NUMBER_OF_WAITING_THREADS
> 132: check(lockCheck, null, 0, // no owner thread
AFAICS you are racing here, as the waiting threads can set `ready` but not yet
have called `wait()`. To know for sure they are in `wait()` you need to acquire
the monitor before checking (and release it again so that you test the no-owner
scenario as intended). But this should probably be done inside
`startWaitingThreads`.
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
line 319:
> 317: try {
> 318: ready = true;
> 319: lockCheck.wait();
Spurious wakeups will break this test. They shouldn't happen but ...
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp
line 65:
> 63:
> 64:
> 65: jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved) {
Nit: there's a double space after jint
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp
line 219:
> 217: }
> 218:
> 219: } // exnern "C"
typo: exnern -> extern
-------------
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1913304471
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510543479
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510540981
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510544636
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510548391
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510549323
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510550756
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510551339
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510551516
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510556424
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510553511
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510557225
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510557802
