RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v25]

[David Holmes]

On Tue, 12 Mar 2024 02:31:43 GMT, Serguei Spitsyn <sspitsyn at 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: addressed minor comments, updated a couple of copyright headers

Changes requested by dholmes (Reviewer).

test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java line 352:

> 350:                     wokeupCount++;
> 351:                     if (!notifiedAll && notifiedCount < wokeupCount) {
> 352:                         wasSpuriousWakeup = true;

This doesn't work. First as you've realized you cannot detect a spurious wakeup when notifyAll is used. But second the `notifiedCount` is incremented on each `notify` whilst the monitor lock is still held - as it must be. So no `wait` can return until the monitor is unlocked. So unless the spurious wakeup occurs before the monitor is acquired at line 254, all notifications must be completed before even a spurious wakeup can cause `wait()` to return - at which point `notifiedCount` must equal ` NUMBER_OF_WAITING_THREADS` which can never be < `wokeupCount`.
Given you can only detect one very specific case of a spurious wakeup, all this extra logic is just adding to the complexity and giving a false impression about what is being checked. To detect actual spurious wakeups here you need to be able to track when each thread was chosen by a `notify()` and there is no means to do that.

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

PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1930117927
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520862516