On Tue, 5 Mar 2024 06:30:20 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: addressed more comments on the fix and new test
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1507:
>
>> 1505: nWait = 0;
>> 1506: for (ObjectWaiter* waiter = mon->first_waiter();
>> 1507: waiter != nullptr && (nWait == 0 || waiter !=
>> mon->first_waiter());
>
> Sorry I do not understand the logic of this line. `waiters` is just a
> linked-list of `ObjectWaiter`s so all we need to do is traverse it and count
> the number of elements.
The loop is endless without this extra condition, so we are getting a test
execution timeout.
The `waiters` seems to be `circular doubly linked list` as we can see below:
inline void ObjectMonitor::AddWaiter(ObjectWaiter* node) {
assert(node != nullptr, "should not add null node");
assert(node->_prev == nullptr, "node already in list");
assert(node->_next == nullptr, "node already in list");
// put node at end of queue (circular doubly linked list)
if (_WaitSet == nullptr) {
_WaitSet = node;
node->_prev = node;
node->_next = node;
} else {
ObjectWaiter* head = _WaitSet;
ObjectWaiter* tail = head->_prev;
assert(tail->_next == head, "invariant check");
tail->_next = node;
head->_prev = node;
node->_next = head;
node->_prev = tail;
}
}
I'll make sure nothing is missed and check the old code again.
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
> line 36:
>
>> 34: * - unowned object without any waiting threads
>> 35: * - unowned object with threads waiting to be notified
>> 36: * - owned object without any waiting threads
>
> You could say "threads waiting" in all cases - it looks odd to reverse it for
> two cases.
Thanks, updated.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512386413
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512391923
