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

[Serguei Spitsyn]

On Mon, 4 Mar 2024 03:20:41 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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
>
> 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;

Thank you for the suggestion. Let me check this.

> 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.

This is the original comment. I also do not quite understand it. It is confusing for sure. I can remove it if Dan does not object.

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

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