On Tue, 22 Sep 2026 13:47:22 GMT, Alan Bateman <[email protected]> wrote:
>> David Holmes has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adjust test per Alan's request
>
> src/hotspot/share/services/threadService.cpp line 1294:
>
>> 1292: // skip
>> 1293: } else {
>> 1294: _locks->push(OwnedLock(depth, OwnedLock::LOCKED,
>> OopHandle(oop_storage(), monitor->owner())));
>
> Maybe you want to emphasize that it's not reported but inverting it to record
> when any of the 4 conditions are met might be easier to read, as it avoids
> the empty branch. It's the same of course, just (to me anyway) a bit easier
> to read.
>
>
> if (_blocker._type != Blocker::WAITING_ON ||
> _blocker._obj.resolve() != monitor->owner() ||
> waiting_monitor == nullptr ||
> (_java_thread != nullptr
> ? waiting_monitor->is_entered(_java_thread)
> : waiting_monitor->owner() ==
> ObjectMonitor::owner_id_from(_thread_h()))) {
> _locks->push(OwnedLock(depth, OwnedLock::LOCKED,
> OopHandle(oop_storage(), monitor->owner())));
> }
I wrote it with the empty branch because I found the inversion much harder to
read and understand. The condition as expressed is:
> if I am waiting on the monitor for this object and I don't own that monitor
> then ignore it; else add it.
The inversion is
> if I am not waiting on the monitor for this object, or I am waiting on it but
> I still own the monitor, then add it.
I prefer to leave as-is thanks.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32977#discussion_r4076973624
