fix(native): add post-native pass timings to result.phases#1491
Conversation
… diff Adds snapshot-pre-bash.sh (PreToolUse Bash) + track-bash-writes.sh (PostToolUse Bash): the pre-hook captures git status --porcelain to a per-worktree temp file before each Bash call; the post-hook diffs the before/after state and appends newly modified or created files to .claude/session-edits.log. This closes the gap where files written by sed -i, printf redirects, tee, heredocs, or build tools (Cargo.lock, lockfiles) were never recorded, causing guard-git.sh to emit false-positive BLOCKED errors. Closes #1457
- clojure.rs: annotate lifetime-anchor assignment to silence false-positive - cfg.rs: remove never-called start_line_of method - complexity.rs: remove never-constructed NotHandled variant; convert irrefutable if-let patterns to plain let destructures - dataflow.rs: remove never-read callee fields from CallReturn/Destructured - incremental.rs: remove never-read lang field from CacheEntry cargo check and cargo clippy both clean after these changes.
Adds .github/workflows/perf-canary.yml — a path-filtered workflow that fires on PRs touching src/extractors/, src/domain/graph/, or crates/** and runs only the incremental-benchmark suite (full build + no-op + 1-file rebuild, both engines). Catches the class of regressions that accumulated invisibly across the Phase 8.x PRs and were only detected at v3.12.0 publish time. The regression guard gains BENCH_CANARY=1 mode: raises thresholds to 50%/100%/150% (standard/noisy/WASM) and skips the build, query, and resolution suites — only incremental checks run. This absorbs shared- runner timing variance while still blocking catastrophic regressions (+98% full build, +1827% 1-file rebuild from v3.12.0). Closes #1433
On incremental builds, runPostNativeCha previously scanned all call→qualified-method edges in the DB (~12ms flat, O(graph size)), even for 1-file changes where no hierarchy or RTA evidence changed. Add two cheap indexed gate queries. Gate A checks whether any changed file introduced a class/interface/trait/struct/record node (hierarchy may have new implementors reachable from unchanged call sites). Gate B checks whether any changed file added a call edge to a class-kind target (RTA set may have grown, enabling previously filtered expansions in unchanged callers). If neither gate fires, restrict the candidate query to src.file IN changedFiles — safe because the hierarchy and instantiated set are unchanged for all other files. Full builds (isFullBuild=true) and cases where either gate fires retain the existing full-scan behaviour. Mirrors the changed-files scoping pattern of runPostNativeThisDispatch. Closes #1441
Times each JS post-pass in tryNativeOrchestrator and exposes the
measurements in BuildResult.phases:
- gapDetectMs — dropped-language gap detection + backfill
- chaMs — CHA expansion (interface dispatch)
- thisDispatchMs — this/super dispatch WASM re-parse (was already
tracked but now properly named alongside the rest)
- reclassifyMs — scoped role re-classification after edge insertion
- techniqueBackfillMs — technique-column UPDATE on native-written edges
Previously only thisDispatchMs was reported, causing wall-clock vs
phaseSum to diverge by 1.1s+ on 1-file rebuilds and making benchmark
regressions undiagnosable from committed history.
Updates update-incremental-report.ts to render the new phases in a
collapsible details block under each engine's 1-file rebuild section.
Closes #1434
|
Claude finished @carlos-alm's task —— View job PR Review: fix(native): add post-native pass timings to result.phases
Phase 1: Root Cause AnalysisProblem Being Solved: The native orchestrator was only reporting Why This Approach: The fix is structurally correct — instead of ad-hoc Risk Assessment: Very low. The change is purely additive — it adds new fields to Phase 2: Configuration Change DetectionNo linting rules, TypeScript flags, test timeouts, or build config are changed. The biome format fix (long type annotation on line ~580 split to multi-line) is a cosmetic cleanup, not a rule relaxation. No concerns. Phase 3: Test IntegrityNo test files are modified. The PR description mentions this closes #1434 (a bug about missing phase timings) but adds no new tests covering the timing reporting. This is a mild gap — there's no regression guard ensuring the new phases don't silently disappear again. However, given that timing fields are internal diagnostics rather than correctness guarantees, this is acceptable for a low-risk observability change. Phase 4: Code Quality
One minor concern —
Biome format fix (lines 580–584): The Phase 5: Backlog Compliance
Critical ConcernsNone. One minor nit (non-blocking): Final Assessment |
Greptile SummaryThis PR adds wall-clock timing instrumentation for the four previously-unmeasured JS post-pass phases in
Confidence Score: 5/5The change is purely additive instrumentation — it wraps existing post-pass calls with performance.now() brackets and surfaces the numbers through an already-optional phases field. No existing call paths or data are mutated. All five post-pass phases are timed consistently with performance.now(), the type changes are backward-compatible optional fields, the report script degrades gracefully for older history entries, and the two issues from the previous review thread have both been addressed. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant O as tryNativeOrchestrator
participant GD as detectDroppedLanguageGap
participant BF as backfillNativeDroppedFiles
participant CHA as runPostNativeCha
participant TD as runPostNativeThisDispatch
participant RC as classifyNodeRoles
participant TB as backfillEdgeTechniquesAfterNativeOrchestrator
participant FMT as formatNativeTimingResult
O->>GD: "t0=performance.now()"
GD-->>O: gap
O->>BF: conditional fullBuild/removed/changed
BF-->>O: done
Note over O: gapDetectMs = now() - t0
O->>CHA: "t0=performance.now()"
CHA-->>O: newEdgeCount, affectedFiles
Note over O: chaMs = now() - t0
O->>TD: "t0=performance.now() inside fn"
TD-->>O: elapsedMs thisDispatchMs, targetIds, affectedFiles
O->>RC: "t0=performance.now() if chaEdges or thisDispatchTargets"
RC-->>O: done
Note over O: reclassifyMs = now() - t0
O->>TB: "t0=performance.now()"
TB-->>O: done
Note over O: techniqueBackfillMs = now() - t0
O->>FMT: PostPassTimings bundle
FMT-->>O: BuildResult phases populated
Reviews (4): Last reviewed commit: "fix: resolve merge conflicts with main" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** Wall-clock time for the prototype-method post-pass (native path only). */ | ||
| protoMethodsMs?: number; |
There was a problem hiding this comment.
protoMethodsMs is declared in BuildResult.phases but is never set by formatNativeTimingResult, never included in the PostPassTimings interface in native-orchestrator.ts, and never rendered in update-incremental-report.ts. It is also absent from the PR description's list of new phases. Because it is optional this causes no runtime errors, but it will appear in IDE autocomplete as a usable field that always reads undefined, which could silently mislead future callers trying to sum phase times.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — removed the orphaned protoMethodsMs field from BuildResult.phases in types.ts. The field was never populated by the orchestrator, report script, or PostPassTimings interface. Commit 45e9af7.
| @@ -933,12 +937,20 @@ async function runPostNativeThisDispatch( | |||
| return { elapsedMs: Date.now() - t0, targetIds, affectedFiles }; | |||
There was a problem hiding this comment.
runPostNativeThisDispatch measures its elapsed time with Date.now() (whole-millisecond integer precision), while all four new timings added in this PR — gapDetectMs, chaMs, reclassifyMs, and techniqueBackfillMs — use performance.now() (sub-millisecond float precision). The values all flow into formatNativeTimingResult and are rounded with .toFixed(1), but thisDispatchMs will always land on a .0 boundary while the others can have fractional parts, making the phase sum slightly inconsistent. If these timings are ever compared programmatically (e.g. to compute an unaccounted residual), the difference in resolution can produce artificial noise.
There was a problem hiding this comment.
Fixed — migrated runPostNativeThisDispatch from Date.now() (integer ms) to performance.now() (sub-ms float) in both the timer start and elapsed calculation. All five post-pass timings in PostPassTimings now use the same timer API and resolution. Commit 45e9af7.
Codegraph Impact Analysis9 functions changed → 6 callers affected across 6 files
|
…ision Remove the `protoMethodsMs` optional field from `BuildResult.phases` that was declared in types.ts but never populated by the orchestrator, report script, or PostPassTimings interface — preventing it from silently appearing as an always-undefined field in IDE autocomplete. Migrate `runPostNativeThisDispatch` from `Date.now()` (integer ms) to `performance.now()` (sub-ms float) so all five post-pass timings in `PostPassTimings` use the same timer API and resolution.
docs check acknowledged — merge resolution only; no new features or API changes to document
1 participant
Times each post-native pass in
tryNativeOrchestratorand exposes them inresult.phases:gapDetectMs— dropped-language gap detection + WASM backfillchaMs— CHA expansion post-pass (interface dispatch → concrete implementations)thisDispatchMs— this/super dispatch WASM re-parse post-passreclassifyMs— scoped role re-classification after edge-writing passestechniqueBackfillMs— technique-column UPDATE on native-written edgesPreviously only
thisDispatchMswas reported, causing wall-clock vs phaseSum to diverge by 1.1s+ on 1-file rebuilds — making benchmark regressions undiagnosable from committed history (as in the v3.12.0 publish-gate failure).Also fixes a pre-existing biome format issue on the
callToMethodschunked query result cast (long inline type annotation on line 580 of the original).Updates
update-incremental-report.tsto render the new phases in a collapsible<details>block under each engine's 1-file rebuild section, so historical benchmark reports show full phase breakdowns.Closes #1434