On Wed, 7 Feb 2024 21:56:01 GMT, Daniel D. Daugherty <dcu...@openjdk.org> wrote:
>> Serguei Spitsyn has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - Merge >> - review: thread in notify waiter list can't be BLOCKED >> - 8324677: Specification clarification needed for JVM TI >> GetObjectMonitorUsage > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1489: > >> 1487: nWait = mon->waiters(); // # of threads in Object.wait() >> 1488: ret.waiter_count = nWant; >> 1489: ret.notify_waiter_count = nWait; > > Please note that the comment on L1487 is accurate. The `waiters` count is > incremented just before the thread that has called `wait()` drops the monitor > and that increased count remains in effect until after the thread has > reentered > the monitor after being notified or the wait timeout has expired. > > The `contentions` count is not incremented in the re-entry path so a thread > that is in `wait()` that has been notified or the wait timeout has expired is > not > counted as a contending thread. > > So if we really want the `waiter_count` field to include threads that have > been > notified or the wait timeout has expired, then we have to add some logic to > carefully increment the `contentions` count. > > This old logic: > ``` ret.waiter_count = nWant + nWait;``` > over counts because it also includes threads in `wait()` that have not yet > been notified and the wait timeout has not expired. However, including > `nWait` is correct for the situation when all of the waiting threads have > been notified or their wait timeouts have expired. > > This also means that `nWait` and the `ret.notify_waiter_count` value are > over counting waiters because that value will include threads that (have > been notified or the wait timeout has expired) AND have not reentered > the monitor. I _think_ we're saying that is NOT what we want. However, > I _think_ that the `nWait` value is fixed below when we're gathering up > the list of waiters. Thank you for reviewing this, Dan! > The contentions count is not incremented in the re-entry path so a thread that is in wait() that has been notified or the wait timeout has expired is not counted as a contending thread. Right. I've discovered this after reading your first comment above this comment. :) I agree with the whole comment. I understand this over counting now. It is a good catch! Will work on fixing this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1482696229
