On Sat, 10 Feb 2024 04:06:37 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:
>
> review: fixed issues in get_object_monitor_usage; extended test coverage in
> objmonusage003
Sorry really struggling to understand this now. We have gone from a simple
miscalculation to apparently doing everything wrong.
IIUC this API does not currently handle virtual threads correctly -i s that the
case? If so I would like to see that fix factored out and done separately
before this fix is done. Thanks.
src/hotspot/share/runtime/javaThread.cpp line 196:
> 194: oop JavaThread::vthread_or_thread() const {
> 195: oop result = vthread();
> 196: if (result == nullptr) {
Is that really sufficient? If so why do we have `is_vthread_mounted` which
checks the continuation?
test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java
line 31:
> 29:
> 30: final static int JCK_STATUS_BASE = 95;
> 31: final static int NUMBER_OF_ENTERER_THREADS = 4;
Nit: ENTERING not ENTERER
test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java
line 33:
> 31: final static int NUMBER_OF_ENTERER_THREADS = 4;
> 32: final static int NUMBER_OF_WAITER_THREADS = 4;
> 33: final static int NUMBER_OF_THREADS = NUMBER_OF_ENTERER_THREADS +
> NUMBER_OF_WAITER_THREADS;
This testing is not sufficient. We have three states of interest:
- entering the monitor directly
- waiting in the monitor via Object.wait
- re-rentering the monitor after being notified (or timed-out or interrupted)
- we need to check all of these conditions in permutations covering zero and
non-zero for each one, and then see that we get the correct counts back from
JVM TI.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1876986745
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487299437
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487304912
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487311675
