-
Notifications
You must be signed in to change notification settings - Fork 10
perf(native): skip backfill on clean incrementals + bench guard tuning #1085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
eb5cd2a
51629d0
fea4fb3
7870166
b42e107
6ace213
fe4b523
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,14 +17,17 @@ import { describe, expect, test } from 'vitest'; | |||||
| // ── Configuration ──────────────────────────────────────────────────────── | ||||||
|
|
||||||
| /** | ||||||
| * Maximum allowed regression (as a fraction, e.g. 0.25 = 25%). | ||||||
| * Maximum allowed regression (as a fraction, e.g. 0.50 = 50%). | ||||||
| * | ||||||
| * Why 25%: The report script warns at 15%, but timing benchmarks have | ||||||
| * natural variance from CI runner load, GC pauses, etc. 25% filters | ||||||
| * noise while still catching the catastrophic regressions we've seen | ||||||
| * historically (100%–220%). Tune this down as benchmarks stabilize. | ||||||
| * Why 50%: The report script warns at 15%, but timing benchmarks have | ||||||
| * substantial natural variance — sub-30ms metrics (no-op rebuild, | ||||||
| * 1-file rebuild) routinely jitter ±10ms from CI runner load, GC pauses, | ||||||
| * and OS scheduling, which translates to ±50%+ on small absolute numbers. | ||||||
| * 50% filters that noise while still catching catastrophic regressions | ||||||
| * we've seen historically (100%–220%). Tune this down as benchmarks | ||||||
| * stabilize. | ||||||
| */ | ||||||
| const REGRESSION_THRESHOLD = 0.25; | ||||||
| const REGRESSION_THRESHOLD = 0.5; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — kept REGRESSION_THRESHOLD at 0.25 so the 30-100ms range stays guarded, and added a NOISY_METRICS set with a separate 0.50 tolerance for the genuinely high-variance sub-30ms metrics (No-op rebuild, 1-file rebuild). A 50ms->74ms (+48%) regression now correctly fails. Fixed in b42e107. |
||||||
|
|
||||||
| /** | ||||||
| * Minimum absolute delta required before a regression is flagged. | ||||||
|
|
@@ -63,22 +66,9 @@ const SKIP_VERSIONS = new Set(['3.8.0']); | |||||
| * Format: "version:metric-label" (must match the label passed to checkRegression). | ||||||
| * Resolution keys use: "version:resolution <lang> precision" or "version:resolution <lang> recall". | ||||||
| * | ||||||
| * - 3.9.0:1-file rebuild — native incremental path re-runs graph-wide phases | ||||||
| * (structureMs, AST, CFG, dataflow) on single-file rebuilds. Documented in | ||||||
| * BUILD-BENCHMARKS.md Notes section with phase-level breakdown. | ||||||
| * | ||||||
| * - 3.9.0:fnDeps depth {1,3,5} — openRepo() always routed queries through the | ||||||
| * native NAPI path regardless of engine selection, so both "wasm" and "native" | ||||||
| * benchmark workers measured native rusqlite open/close overhead (~27ms vs | ||||||
| * ~10ms with direct better-sqlite3). Fixed by wiring CODEGRAPH_ENGINE through | ||||||
| * openRepo(); v3.10.0 benchmarks will reflect the corrected measurements. | ||||||
| * | ||||||
| * - 3.9.1:1-file rebuild — continuation of the 3.9.0 regression; native | ||||||
| * incremental path still re-runs graph-wide phases on single-file rebuilds. | ||||||
| * Benchmark data shows 562 → 767ms (+36%). Same root cause as 3.9.0 entry. | ||||||
| * | ||||||
| * - 3.9.2:Full build — NativeDbProxy overhead causes native full build to | ||||||
| * regress from 5206ms to 9403ms (+81%). Fix tracked in PR #906. | ||||||
| * Entries fire only when `latest.version` matches the prefix, so once a | ||||||
| * version is no longer the latest in committed history its entries become | ||||||
| * dead weight and should be removed (last pruned: 3.9.0/3.9.1/3.9.2). | ||||||
| * | ||||||
| * - 3.9.6:Build ms/file / 3.9.6:No-op rebuild — WASM full build regressed | ||||||
| * (#1036) when PR #1016 expanded AST_TYPE_MAPS from 3 to 23 languages, | ||||||
|
|
@@ -98,19 +88,26 @@ const SKIP_VERSIONS = new Set(['3.8.0']); | |||||
| * cloned", skipping every Haskell file with constructors. Fixed by gating | ||||||
| * with `Object.hasOwn` (#1039). Benchmarks captured before the fix landed; | ||||||
| * will reclear in v3.9.7+ data. | ||||||
| * | ||||||
| * - 3.10.0:No-op rebuild — small (~3–7ms / ~25–35% local) real regression | ||||||
| * amplified by CI variance on a sub-30ms metric. Verified by running | ||||||
| * the v3.9.6 source+binary against this repo: steady-state 14–19ms vs | ||||||
| * HEAD's 18–22ms. The JS-side fast-skip pre-flight (#1064) was *not* | ||||||
| * the cause — disabling it leaves the orchestrator-only path at the | ||||||
| * same ~20ms. Likely contributors: the extra is_supported_extension | ||||||
| * filter in detect_removed_files (#1070), the larger file_hashes row | ||||||
| * set after #1069 stores symbol-less files, and tree-sitter 0.24→0.25. | ||||||
| * Each is a few hundred microseconds; aggregated they explain the | ||||||
| * local delta. CI's +120% reflects sub-30ms metric noise floor on | ||||||
| * shared runners — not a 2x slowdown. | ||||||
| */ | ||||||
| const KNOWN_REGRESSIONS = new Set([ | ||||||
| '3.9.0:1-file rebuild', | ||||||
| '3.9.0:fnDeps depth 1', | ||||||
| '3.9.0:fnDeps depth 3', | ||||||
| '3.9.0:fnDeps depth 5', | ||||||
| '3.9.1:1-file rebuild', | ||||||
| '3.9.2:Full build', | ||||||
| '3.9.6:Build ms/file', | ||||||
| '3.9.6:No-op rebuild', | ||||||
| '3.9.6:Query time', | ||||||
| '3.9.6:resolution haskell precision', | ||||||
| '3.9.6:resolution haskell recall', | ||||||
| '3.10.0:No-op rebuild', | ||||||
| ]); | ||||||
|
|
||||||
| /** | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a developer adds a new
.hcl,.scala, or.swiftfile (any extension outside Rust'sfile_collector) on an incremental build that has no other removals, the Rust orchestrator reportsremovedCount=0andchangedCount=0(it never sees the file). The guard therefore skips backfill, leaving the new file absent from the graph until the next full rebuild with no warning.Pre-#1069, backfill ran only on full builds, so this gap existed then too — but #1069 intentionally closed it by running backfill on every incremental. This PR re-opens that gap for the "new unsupported file, zero Rust-side activity" case. The perf win is real, but the condition could be broadened to
result.isFullBuild || removedCount > 0 || (result.changedCount ?? 0) > 0to at least cover incrementals where any file activity was detected, reducing (though not fully closing) the window.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — broadened the guard to fire when
result.changedCount > 0as well, so any orchestrator-detected file activity now triggers backfill. This narrows the gap meaningfully (most incrementals do have changedCount > 0).The residual case you identified (brand-new unsupported-extension file added on an otherwise-quiet incremental, where the orchestrator sees neither removals nor changes) requires JS-side detection of files the Rust collector doesn't see — tracked as a follow-up in #1091. Fixed in 6ace213.