JDK-8324677 : incorrect implementation of JVM TI GetObjectMonitorUsage
  • Type: CSR
  • Component: hotspot
  • Sub-Component: jvmti
  • Priority: P4
  • Status: Provisional
  • Resolution: Unresolved
  • Fix Versions: 23
  • Submitted: 2024-01-25
  • Updated: 2024-02-13
Related Reports
CSR :  
JDK-8247972 - incorrect implementation of JVM TI GetObjectMonitorUsage
Relates :  
JDK-8325314 - Release Note: incorrect implementation of JVM TI GetObjectMonitorUsage
Description
Summary
-------

The JVMTI `GetObjectMonitorUsage` implementation gives incorrect information about threads waiting to enter or re-enter the ObjectMonitor which does not match the function spec.

Problem
-------

The JVMTI `GetObjectMonitorUsage` returns the following data structure:
```
    typedef struct {
        jthread owner;
        jint entry_count;
        jint waiter_count;
        jthread* waiters;
        jint notify_waiter_count;
        jthread* notify_waiters;
    } jvmtiMonitorUsage;
```

The following two fields in this structure are specified as:
```
waiter_count  [jint] The number of threads waiting to own this monitor
waiters       [jthread*] The waiter_count waiting threads
```

The `waiters` field 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.


Solution
-------

The solution is to align the implementation with the spec, so that in the `waiters` field only contains the threads waiting to enter/re-enter the monitor.

The implementation of the JDWP command `ObjectReference.MonitorInfo` has to be adjusted to keep the original behavior.

The bug needs a Release Note.

The following JVMTI vmTestbase tests have to be fixed to adopt the modified behavior:
```
vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001/TestDescription.java
vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003/TestDescription.java
```

The following JVMTI JCK tests have to be fixed to adopt the modified 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
```

--------



Specification
-------------

The JVMTI `GetObjectMonitorUsage` spec is not being changed and can be found here:

  https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#GetObjectMonitorUsage
Comments
[~sspitsyn], I don't mind if this the complete fix here is one under one bug or two, but I think [~alanb]'s suggestion could have less overhead. Please Finalize the request once one bug or two has been decided; thanks.
13-02-2024
I agree changing the GetObjectMonitorUsage implementation is match the spec is the best route. However, I assumed the JDWP spec would be adjusted at the same time, Sure, it's okay to do it as separate issue but it would be helpful to have the discussion in the one PR and one CSR rather than two.
10-02-2024
[~darcy] A release note has already been created. I'm not sure what is not clear about changes for these two fields. Before the fix the `waiters` array returns the `waiting_count` threads waiting to enter/re-enter the object monitor and additionally threads waiting to be notified in a `java.lang.Object.wait()` call. After the fix the `waiters` array returns the `waiting_count` threads waiting to enter/re-enter the object monitor. The threads waiting to be notified in a `java.lang.Object.wait()` call are dropped from the returned array. Please, let me know if this is clear or NOT, or if you want me to update the CSR description with this.
07-02-2024
> As I understand, it is okay with you to address it separately. [~sspitsyn] yes fine. Thanks.
07-02-2024
Moving back to Provisional, not Approved. [~sspitsyn], please state more explicitly what information the fields hold today vs what they will hold after this change. I've updated the parent issue to indicate a release note is required.
06-02-2024
[~darcy] I agree with David's opinion on this. Also, the JVM TI GetObjectMonitorUsage() is a debugging feature that is used by the JDWP ObjectReference.MonitorInfo command. The implementation was adjusted to keep the existing behavior of this JDWP command. There is very low probability that the function GetObjectMonitorUsage() is really used by any other existing JVM TI agents.
06-02-2024
Thank you for review, David! I've filed a separate issue on the JDWP spec (this was on my TODO list): JDK-8325272: JDWP ObjectReference.MonitorInfor command spec needs a clarification As I understand, it is okay with you to address it separately.
06-02-2024
I think changing the specification to match the implementation will simply highlight exactly how confused things are as we can't change the names of the variables, and the updated descriptions would not make any sense in relation to the variable names: ``` waiter_count [jint] The number of threads waiting to own this monitor, or waiting to be notified notify_waiter_count [jint] The number of threads waiting to be notified by this monitor, including those already notified (or interrupted, or timed-out) and waiting to re-own the monitor ``` I also think the fact we have never had a bug report (AFAIK) pointing out the errant behaviour means that this is a little used part of the API and the answer is not that critical to normal operations.
06-02-2024
Moving to Provisional, not Approved. By default, in a situation like this where the long-standing behavior contradicts the specification, we change the specification to match the behavior. Is that not feasible here or otherwise ill-advised?
05-02-2024
I've added myself as a Reviewer too. It will be satisfying to have the implementation match the specification, at least at the JVMTI level, though it will be an observable change and so will also need a Release Note IMO. Thanks. However, I think a clarification to the JDWP spec is also in order as it simply states: waiters The number of threads that are waiting for the monitor 0 if there is no current owner with no explanation of exactly what "waiting for the monitor" means. That said there is a problem with the interpretation that it means "waiting to enter, or waiting to be notified" as that conflicts with the statement "0 if there is no current owner" because we can have no current owner but still have multiple threads waiting to be notified. But that said I think any mention of "current owner" here is a mistake and not relevant to what "waiters" is actually trying to report: it will be zero if there are no waiters, regardless of whether there is currnetly an owner or not. So this part of the JDWP spec needs fixing too. Separate issue is fine for that of course.
02-02-2024
Dan, thank you for prompt review!
02-02-2024