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

[Daniel D. Daugherty]

On Thu, 8 Feb 2024 10:34:14 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1521:
>> 
>>> 1519:       // we have contending and/or waiting threads
>>> 1520:       if (nWant > 0) {
>>> 1521:         // we have contending threads
>> 
>> This block includes this logic:
>> 
>>         // get_pending_threads returns only java thread so we do not need to
>>         // check for non java threads.
>>         GrowableArray<JavaThread*>* wantList = Threads::get_pending_threads(tlh.list(), nWant, (address)mon);
>>         if (wantList->length() < nWant) {
>>           // robustness: the pending list has gotten smaller
>>           nWant = wantList->length();
>>         }
>> 
>> `Threads::get_pending_threads()` only returns threads where the
>> `current_pending_monitor` field is set for the specific monitor. That
>> happens only on contended enter and does not happen on contended
>> re-enter so this logic will already strip out any threads in `wait()` that
>> have not been notified and have not had their wait timers expire.
>> It will also strip out any waiters that have been notified or had
>> their wait timeouts expire.
>> 
>> This means even if we fix the reenter code to increment the contentions
>> field, this logic will reduce that `nWant` value. Of course, the way around
>> that is to also update the reenter code to properly set the `current_pending_monitor`
>> field and then the reentering threads won't be filtered out...
>
> Yes, I've figured this out now. Thank you for pointing to it.
> It feels, the counts can be calculated correctly without touching the implementation of `current_pending_monitor()`, `current_waiting_monitor()`, `_contensions` and `_waiters`.
> At least, I'll try to fix it locally in the `JvmtiEnvBase::get_object_monitor_usage()`.
> Please, let me know what do you prefer.

I don't have a preference (yet). Like what David suggested, I think we need to
determine the list of thread and monitor interaction scenarios that want to
exercise with this API. Once we're happy that those scenarios represent the
complete list of possible combinations we should double check that against the
API spec to make sure that those scenarios cover the API spec. Finally, we need
to make sure that we have a test that covers all those scenarios.

It has been a long, long time since I've looked at those NSK tests... :-(

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1483168890