[Code Health] Warn when task fanin/fanout exceeds 16 during dependency construction#1298
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a Changesa2a3 dense fanin/fanout diagnostics
a5 dense fanin/fanout diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Producer as Producer Task
participant Orchestrator as append_fanin_or_fail
participant Counters as Fanout/Fanin Counters
participant Scheduler as wire_task
participant Shutdown as SchedulerContext::shutdown
Producer->>Orchestrator: claim producer fanout_count
Orchestrator->>Counters: compute fanout_now, update high-water/over_count
Orchestrator-->>Orchestrator: LOG_WARN if threshold crossed
Scheduler->>Counters: check wfanin vs threshold
Scheduler-->>Scheduler: LOG_WARN on new high-water
Shutdown->>Counters: sum fanin_over_count across rings
Shutdown-->>Shutdown: log summary if total > 0
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dense-dependency diagnostics for both fanin and fanout across the a2a3 and a5 runtimes, logging warnings when dependency degrees exceed a threshold of 16 and printing summaries upon completion. It also adds a new test case DenseFanoutFanin to verify this behavior. The reviewer feedback highlights two main issues: first, the global static variables used for fanout diagnostics are only reset in mark_done(), which can cause state leakage across runs if a run aborts or fails; second, inline logging (LOG_WARN) within header-inline methods compiled for device should be avoided and delegated to cold-path helpers to prevent code bloat on the hot path.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
yanghaoran29
force-pushed
the
codehealth/warn-dense-fanin-fanout
branch
from
d0803a8 to
1350ecc
Compare
…ruction Very large fanin/fanout is a code-health smell (see hw-native-sys#959): it makes the dependency graph hard to reason about and can hide an unexpectedly dense wiring shape. Nothing flagged it before. Closes hw-native-sys#1261. Add a two-part, bounded diagnostic, split by the phase where each degree is naturally constructed: - Fanout (pto_orchestrator.cpp, append_fanin_or_fail): the orchestrator increments each producer's fanout_count per claimed consumer. Emit an immediate WARN only on a new high-water above the threshold (computed under fanout_lock, logged after unlock — no logging in the critical section), and count each producer once at the 16->17 crossing. - Fanin (pto_scheduler.h, wire_task): scheduler thread 0 wires the task's fanin edges; warn on a new per-ring high-water and count each task. The WARN is emitted through report_dense_fanin_warning(), defined out-of-line in pto_scheduler.cpp, so the device-compiled, header-inline wire_task hot path carries no logging code. At program end, emit one summary line per metric: - "=== [Orchestrator] tasks with fanout>16: N ===" in mark_done() - "=== [Scheduler] tasks with fanin>16: N ===" in shutdown() (thread 0) The high-water gate keeps the AICPU device log from flooding on the hot path (codestyle rule 7). Both degree counters live as per-run state (fanout on PTO2OrchestratorState, fanin on RingSchedState) and are reset at the start of each run in reset_for_reuse — so an aborted run that skips mark_done/shutdown cannot leak them into the next run on the same loaded AICPU binary. Threshold PTO2_DEP_DEGREE_WARN_THRESHOLD=16. Purely diagnostic — no behavior change, no new env/macro gate. Mirrored to a2a3 and a5. Add a dummy_task st case (DenseFanoutFanin / case=4): one producer fanned out to 18 dummy barriers, then one consumer depending on all 18, so both degrees exceed the threshold. Verified on a2a3sim + a5sim that the WARNs fire only on new maxima (17/18/19) and the summaries report 1/1.
yanghaoran29
force-pushed
the
codehealth/warn-dense-fanin-fanout
branch
from
1350ecc to
33ce682
Compare
1 participant
Summary
Closes #1261. Very large fanin/fanout is a code-health smell (cf. #959) — it makes the dependency graph hard to reason about and can hide an unexpectedly dense wiring shape. Nothing flagged it before.
This adds a two-part, bounded diagnostic, split by the phase where each degree is naturally constructed:
1. Immediate WARN during execution (high-water gated)
Per codestyle rule 7 (never flood the AICPU device log on the hot path), each metric warns only on a new high-water max above the threshold:
pto_orchestrator.cpp::append_fanin_or_fail, where the orchestrator increments each producer'sfanout_countper claimed consumer. The value is captured underfanout_lockand logged after unlock (no logging in the critical section).pto_scheduler.h::wire_task, where scheduler thread 0 wires a task's fanin edges (wfanin), gated on a per-ring high-water.2. End-of-run summary
Every task/producer over the threshold is counted silently during the run, then one line per metric is emitted at teardown:
=== [Orchestrator] tasks with fanout>16: N ===inmark_done()=== [Scheduler] tasks with fanin>16: N ===inSchedulerContext::shutdown()(thread 0)Counters reset per run. Threshold
PTO2_DEP_DEGREE_WARN_THRESHOLD = 16. Purely diagnostic — no behavior change, no new env/macro gate. Mirrored to a2a3 and a5.Test
Adds a
dummy_taskst case (DenseFanoutFanin,case=4): one producer fanned out to 18 dummy barriers, then one consumer depending on all 18, so both degrees exceed the threshold.Verified on
a2a3sim+a5sim(both pass):18 consumers → only 3 WARN lines (one per new max) confirms the flood guard. Summaries need
--log-level v0; the WARNs show at the default level.Notes
PTO2_FANIN_INLINE_CAPis 64, so 16 is an advisory policy threshold, not a structural boundary.dcmiinit fails, arch precheck refuses); the sim runs exercise the identical orchestrator/scheduler code paths.🤖 Generated with Claude Code