fix(native): add post-native pass timings to result.phases#1491
fix(native): add post-native pass timings to result.phases#1491carlos-alm wants to merge 2 commits into
Conversation
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 for all five JS post-passes in
Confidence Score: 5/5Safe to merge. All five new timers use performance.now() consistently, both the normal and early-return paths supply every required field, and the report renderer degrades gracefully for older benchmark entries lacking phase data. The change is additive: new timing brackets around existing logic, a widened function signature, and five optional type fields. No existing behavior is altered and both call sites of formatNativeTimingResult have been updated. No files require special attention. src/types.ts has a minor field ordering inconsistency but no functional impact. Important Files Changed
Sequence DiagramsequenceDiagram
participant O as tryNativeOrchestrator
participant GD as detectDroppedLanguageGap / backfill
participant CHA as runPostNativeCha
participant TD as runPostNativeThisDispatch
participant RC as classifyNodeRoles
participant TB as backfillEdgeTechniques
O->>GD: "gapDetectStart = performance.now()"
GD-->>O: gap
O->>O: conditional backfillNativeDroppedFiles
O->>O: "gapDetectMs = performance.now() - gapDetectStart"
O->>CHA: "chaStart = performance.now()"
CHA-->>O: "{newEdgeCount, affectedFiles}"
O->>O: "chaMs = performance.now() - chaStart"
O->>TD: (performance.now() timer inside)
TD-->>O: "{elapsedMs: thisDispatchMs, targetIds, affectedFiles}"
O->>RC: "reclassifyStart = performance.now() (if edges added)"
RC-->>O: roles updated
O->>O: "reclassifyMs = performance.now() - reclassifyStart"
O->>TB: "techniqueBackfillStart = performance.now()"
TB-->>O: done
O->>O: "techniqueBackfillMs = performance.now() - techniqueBackfillStart"
O->>O: "formatNativeTimingResult({gapDetectMs,chaMs,thisDispatchMs,reclassifyMs,techniqueBackfillMs})"
Reviews (3): Last reviewed commit: "fix(native): remove orphaned protoMethod..." | 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 Analysis10 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.
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