On Tue, 22 Sep 2026 07:19:17 GMT, David Holmes <[email protected]> wrote:
>> The `GetThreadSnapshotHandshakeClosure` was added to support virtual threads >> in thread dumps. It is very similar to the logic used in the old >> safepoint-based thread dump. >> `GetThreadSnapshotHandshakeClosure::detect_locks` uses the raw >> `javaVFrame::monitors()` method rather than using the `locked_monitors()` >> method which already filters out some monitors including those for which >> `wait()` has been called. It uses the raw `monitors()` list because it wants >> to process eliminated compiled monitors itself, and they are already removed >> from `locked_monitors()`. But that means it should be doing its own >> filtering of monitors that are being waited-on so they are not reported as >> locked. This seems to have been an oversight with the original >> implementation. >> >> The logic is somewhat more complicated, particularly in the virtual thread >> case, because there are a number of points where we can dump the stack >> between `wait0` appearing as the top frame, and the point where we actually >> release the monitor. It would be wrong to hide the monitor from the "locked" >> section in that case. So we can still report that a thread is waiting-on a >> particular object and that it has locked that object, as that is the actual >> state of things. But the code no longer reports a released monitor as locked >> by the waiting thread. >> >> Note that `javaVFrame::locked_monitors` is also potentially imprecise in its >> reporting. It will report the monitor as locked until >> `current_waiting_monitor()` is set, and then elide it. However, as this code >> is only used when reporting platform threads this imprecision is not >> observable as a platform thread will not respond to a stack dump request in >> between the transition to in-VM (for the native call) and the release of the >> monitor. >> >> I also fixed a pre-existing typo whilst in this code. >> >> Testing >> - tiers 1-3 >> - tier 5-svc >> - updated tests in the PR >> >> Thanks >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust test per Alan's request Latest version looks okay, just one suggestion in comments. 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()))); } ------------- Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/32977#pullrequestreview-5278956272 PR Review Comment: https://git.openjdk.org/jdk/pull/32977#discussion_r4072348453
