[None][perf] Cut per-iteration executor bookkeeping in the hang detector and profiler - #17531
[None][perf] Cut per-iteration executor bookkeeping in the hang detector and profiler#17531pranav-nvidia wants to merge 3 commits into
Conversation
checkpoint() cancelled the pending watchdog task and scheduled a new one, and both calls post a call_soon_threadsafe, so the executor loop woke the detector thread several times per iteration to re-arm a 300s watchdog. Keep one long-lived watcher and move a monotonic deadline instead. The timeout is now measured from the checkpoint call rather than from when the detector thread gets to process it. Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com>
The start events were recorded on every executor iteration while the read side is already gated on print_log or enable_iter_perf_stats, so with neither enabled the record calls were pure overhead. Gate the record on the same condition. Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #65606 [ run ] triggered by Bot. Commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change replaces per-detection hang tasks with one persistent watcher and a shared deadline. It adds tests for watcher reuse, disarming, race handling, and callback failures. Profiler timing capture is gated by enabled statistics. ChangesPersistent hang monitoring
Profiler timing capture
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HangDetector
participant _watch
participant _report_hang
HangDetector->>_watch: schedule persistent watcher
_watch->>HangDetector: observe shared deadline
_watch->>_report_hang: report elapsed deadline
_report_hang-->>_watch: return or raise
_watch->>HangDetector: continue monitoring
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unittest/_torch/executor/test_hang_detector_kill.py (1)
70-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct lifecycle tests for
HangDetector.Coverage summary:
- Added four tests and modified
test_status_provider_errors_are_logged.- CI registration is sufficient.
l0_sanity_check.ymllists the module, andl0_h100.ymlandl0_cpu.ymlcover its directory.- No QA-list entry is required for this unit-test change.
- Coverage is insufficient for direct
cancel_task(),stop(), repeatedstart(), and detection latency afterpause()resumes. Add focused tests for these cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/executor/test_hang_detector_kill.py` around lines 70 - 169, Add focused lifecycle tests for HangDetector covering direct cancel_task(), stop(), repeated start() calls, and detection latency after pause() resumes. Verify each method’s expected task/deadline state and that resumed monitoring detects a subsequent hang within the configured timeout, using the existing test patterns and symbols such as hd.task, hd.pause(), and hd.checkpoint().Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_torch/pyexecutor/hang_detector.py`:
- Around line 360-379: Move the self._deadline = math.inf reset into the
self.lock region in HangDetector.start(), placing it immediately before
self.active = True. Remove the later reset after the lock so a concurrent
checkpoint() cannot arm the deadline and have it overwritten.
- Around line 401-406: Bound the sleep interval in the active watcher loop
around _deadline so disarmed states with an infinite remaining duration do not
sleep for the full timeout. Clamp asyncio.sleep to a fraction of self.timeout
while preserving deadline-based sleeping when armed, ensuring a checkpoint that
arms the detector during sleep is observed within the intended timeout latency.
---
Nitpick comments:
In `@tests/unittest/_torch/executor/test_hang_detector_kill.py`:
- Around line 70-169: Add focused lifecycle tests for HangDetector covering
direct cancel_task(), stop(), repeated start() calls, and detection latency
after pause() resumes. Verify each method’s expected task/deadline state and
that resumed monitoring detects a subsequent hang within the configured timeout,
using the existing test patterns and symbols such as hd.task, hd.pause(), and
hd.checkpoint().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aad36bb0-6552-4098-9e32-abdac3dd9174
📒 Files selected for processing (3)
tensorrt_llm/_torch/pyexecutor/hang_detector.pytensorrt_llm/_torch/pyexecutor/py_executor.pytests/unittest/_torch/executor/test_hang_detector_kill.py
start() published `active` inside the lock but reset `_deadline` after releasing it. A checkpoint from the executor thread in that window armed the deadline, and the reset then erased it, leaving the loop unwatched until the next checkpoint. Store the disarmed deadline inside the lock, before `active` goes true. Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #65665 [ run ] triggered by Bot. Commit: |
|
PR_Github #65606 [ run ] completed with state |
|
PR_Github #65665 [ run ] completed with state
|
|
/bot run |
|
PR_Github #66000 [ run ] triggered by Bot. Commit: |
|
PR_Github #66000 [ run ] completed with state
|
|
/bot run |
|
PR_Github #66044 [ run ] triggered by Bot. Commit: |
|
PR_Github #66044 [ run ] completed with state
|
|
/bot run |
|
PR_Github #66087 [ run ] triggered by Bot. Commit: |
|
PR_Github #66087 [ run ] completed with state |
zhaoyangwang-nvidia
left a comment
zhaoyangwang-nvidia
left a comment
There was a problem hiding this comment.
Approve with nits.
| # once. A checkpoint racing this branch installs a newer deadline; | ||
| # clearing that would leave the watcher alive but permanently | ||
| # disarmed, which is the failure this watchdog exists to catch. | ||
| if self._deadline == deadline: |
There was a problem hiding this comment.
if self._deadline == deadline: self._deadline = math.inf is a non-atomic compare-and-set — the load/compare and the store are separate bytecodes, so a checkpoint() from the executor thread landing between them is still erased. That is the case the comment rules out, and test_a_checkpoint_racing_the_disarm_is_not_erased doesn't cover it either: it injects from _watch's own time.monotonic() call, i.e. between the local read and the comparison, not between the comparison and the store. The practical impact is small (a lapse only reaches here after timeout without a checkpoint, and the next checkpoint re-arms within an iteration), but since this is off the hot path, could these two lines take self.lock, or the comment be softened to best-effort?
| self.task = asyncio.run_coroutine_threadsafe(self._detect_hang(), self.loop) | ||
| self._deadline = time.monotonic() + self.timeout | ||
|
|
||
| def cancel_task(self): |
There was a problem hiding this comment.
cancel_task() no longer cancels a task — it only disarms the deadline, as the new docstring correctly says, so the name now points at the old design. Note it can't simply be renamed: run_precheck.py calls it and test_cache_transceiver_precheck_run.py::test_hang_detector_surface asserts the attribute exists. Consider adding disarm() as the real name and keeping cancel_task = disarm as an explicit compatibility alias.
| @contextmanager | ||
| def pause(self): | ||
| """Pause hang detection in scope.""" | ||
| self._deadline = math.inf |
There was a problem hiding this comment.
pause() previously called cancel_task(); inlining self._deadline = math.inf here makes four places that write the sentinel. pause() runs twice per iteration and is already orders of magnitude cheaper than the task cancel it replaces, so the call shouldn't affect the numbers — keeping it leaves "what disarming means" (and any future change to it, e.g. taking the lock) in one place.
4 participants
Summary
The executor loop's hang-detector heartbeat did cross-thread work on every call.
checkpoint()cancelled the pending watchdog task and scheduled a new one; bothFuture.cancel()andrun_coroutine_threadsafe()post acall_soon_threadsafe, waking the detector's event-loop thread. With three checkpoints per iteration (loop top plus bothpause()exits) that is roughly six cross-thread wakeups and three Task lifecycles per iteration, to re-arm a watchdog that will not fire for 300 s.This keeps one long-lived watcher and moves a monotonic deadline instead, so
checkpoint()becomes one clock read and one float store.A second, smaller commit gates the profiler start-event record on the same condition the read side already uses (
print_log or enable_iter_perf_stats), so events nobody reads are not recorded.This is not Whisper-specific. It is a fixed per-iteration cost, so it matters in proportion to how short an iteration is — every host-bound workload pays it.
Measurements
whisper-tiny, SM120, fp16 greedy, 32-clip librispeech, median of 5 blocks x 5 iters after 10 warmup, 3 interleaved A/B rounds with medians reported.
Both deltas exceed the observed run-to-run range (2.6-5.6 %). WER is 0.1250 in every arm.
Measured separately, the hang-detector commit accounts for +14.9 % / +11.6 % and the profiler commit for +2.4 % / +3.3 % (the latter is inside the noise band at bs32).
Behavioural notes
The watchdog's fire semantics are preserved deliberately, since its failure mode is a silent multi-rank hang:
start()leaves the detector disarmed; the firstcheckpoint()arms it, as before. The start-to-first-checkpoint window is not hang-eligible.on_detectedis the cross-rank hard kill and can itself fail on a degraded job; a watchdog that quietly stopped watching would be the exact failure it exists to catch.checkpoint()racing the watcher's disarm is not erased — the watcher only clears the deadline it observed lapse.start()ignores a call on an already-active detector, so there is no path to two watchers sharing one deadline and propagating two hard kills.pause()is unchanged relative tomain, including its behaviour when nested.One deliberate difference: the timeout is now measured from the
checkpoint()call rather than from when the detector thread processes it. If a caller holds the GIL past the timeout, this reports on release wheremainwould grant a fresh interval. Restoringmain's arm epoch would require arming on the detector thread, which is precisely the cross-thread work this change removes.Tests
Four tests added to
tests/unittest/_torch/executor/test_hang_detector_kill.py, each pinning one production behaviour and each verified to fail when that behaviour is reverted: single-watcher reuse, disarmed-until-first-checkpoint, watcher survival across a raising callback, and the checkpoint-vs-disarm race.Dev Engineer Review
checkpoint(),cancel_task(), andpause()update the deadline without repeated task cancellation or scheduling.print_logorenable_iter_perf_statsis enabled.checkpoint()call.QA Engineer Review
tests/unittest/_torch/executor/test_hang_detector_kill.py._report_hang().test-db/orqa/test-list entry was added.