Repository navigation
perf(kernel): load only dependency-reachable rows when rebuilding the alignment projection - #205
Conversation
…ing the alignment projection Every commit that inserts a decision rebuilds the projection, and the rebuild loaded and hashed every decision and observation in the store, which was half of a 128-operation commit and grew with the store. Only the observations an implements dependency names and the decisions reachable from it through supersession contribute rows, so those are the rows read.
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 107 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -178,11 +210,12 @@ fn load_alignment_input( | |||
| AND (e.invalidated_commit_seq IS NULL OR ?1<e.invalidated_commit_seq) | |||
| ) | |||
| FROM decisions d | |||
| WHERE created_commit_seq<=?1", | |||
| WHERE created_commit_seq<=?1 | |||
| AND object_id IN (SELECT value FROM json_each(?2))", | |||
| ) | |||
| .map_err(|_| KernelError::Io)?; | |||
| let rows = statement | |||
| .query_map([requested], |row| { | |||
| let loaded = statement | |||
| .query_map(rusqlite::params![requested, ids], |row| { | |||
| Ok(( | |||
| row.get::<_, String>(1)?, | |||
| DecisionHistory { | |||
| @@ -194,11 +227,33 @@ fn load_alignment_input( | |||
| )) | |||
| }) | |||
| .map_err(|_| KernelError::Io)? | |||
| .collect::<rusqlite::Result<HashMap<_, _>>>() | |||
| .collect::<rusqlite::Result<Vec<_>>>() | |||
| .map_err(|_| KernelError::Io)?; | |||
| rows | |||
| }; | |||
| let observations = { | |||
| let mut next = Vec::new(); | |||
| for (object_id, history) in loaded { | |||
| if let Some(successor) = history.superseded_by.as_deref() { | |||
| if !decisions.contains_key(successor) && !frontier.iter().any(|id| id == successor) | |||
| { | |||
| next.push(successor.to_string()); | |||
| } | |||
| } | |||
| decisions.insert(object_id, history); | |||
| } | |||
| next.sort_unstable(); | |||
| next.dedup(); | |||
| next.retain(|id| !decisions.contains_key(id)); | |||
| frontier = next; | |||
| } | |||
There was a problem hiding this comment.
Nit / design consideration (not a correctness issue — I traced the frontier expansion against resolve_decision's traversal and it matches, including cycle handling and the invalidated_commit_seq<=?1 gating).
This walks the supersession chain with one round trip per level (prepare_cached + query_map per iteration of while !frontier.is_empty()). For a dependency whose decision has been revised many times, that's O(chain depth) sequential statement executions instead of the single query the old code used. Since SQLite is embedded this is cheap per call, but it's worth confirming the benchmark fixtures (commit/128 etc.) actually exercise a deep supersession chain rather than only wide/shallow dependency sets — otherwise this cost is invisible in the measured numbers but could show up on a long-lived store where a hot decision has been superseded dozens of times.
If that turns out to matter, a WITH RECURSIVE CTE over decisions could compute the reachable set in one query while still only touching dependency-reachable rows.
There was a problem hiding this comment.
Verified, and the gap is wider than suspected: the bench suite doesn't just lack deep chains — kernel_routes.rs creates no implements dependencies and no supersessions at all (insert_decision_op/insert_observation_op emit neither, and rg supersede crates/mc-module/benches/ is empty). So the frontier loop runs zero iterations in every measured scenario; the reported wins come entirely from skipping the full-table load when the dependency set is empty or small.
Declining the WITH RECURSIVE rewrite in this PR, for two reasons:
- Cost shape. Each frontier round is one cached statement over
json_each, batched across all chains — rounds = max chain depth, not total chain nodes. An in-process cached statement execution is ~µs, so even depth 50 costs tens of µs against the ~12.6 ms this PR shaves off commit/128. The old code's single query was O(store size); the new per-depth cost only loses to it on a pathological store (tiny table, very long chain). - Methodology. The stack's ABBA baseline pins the bench suite at the first PR of the stack; adding a deep-chain fixture here would break cumulative-ratio comparability. And rewriting the query now would re-encode the
invalidated_commit_seq<=?1CASE gating and theevidence_metaEXISTS eligibility check inside a recursive CTE with no measurement showing it's needed.
Filed a follow-up (beads P2) to add a deep-chain + wide/shallow-control commit fixture after the stack lands, and to evaluate the recursive CTE if depth shows up in the numbers.
ReviewFocused on Correctness: I traced the new frontier-expansion loop against
Performance consideration (left as an inline comment): the supersession-chain walk now issues one query per chain level instead of one query total. For deep chains this trades the old single full-table load for several small round trips — likely still a net win given SQLite is in-process, but worth confirming the benchmarked scenarios include a deep chain, not just wide dependency sets, since that's the case this loop is most sensitive to. No security issues found (read-only projection rebuild, transaction-scoped, no user-controlled SQL text). No test files changed, consistent with the PR description that this is a pure internal read-path optimization with unchanged wire/receipt semantics. |
… search The first frontier round holds every distinct decision an implements dependency names, and each superseded row scanned the whole frontier to decide whether its successor was already queued. The frontier is sorted and deduplicated before each round, so a binary search answers the same question without a cost that grows with the product of the two counts.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by gemini-3.8-flash · Input: 126.8K · Output: 68.8K · Cached: 582.8K |
1 participant
Every commit that inserts a decision rebuilds the alignment projection, and
load_alignment_inputloaded and hashed every decision and every observation in the store to do it: 48% of a 128-operation commit after the statement cache, and growing with the store. Only observations named by animplementsdependency and the decisions reachable from those dependencies through supersession can produce rows, so the loader now reads the dependencies first, then the reachable decisions in frontier rounds overjson_each, then the named observations.derive_alignment, the generation watermark, and the corrupt-row detection for referenced observations are unchanged; a store with no alignment dependency loads no decision at all.Measured (marginal): commit/1 1.46 → 0.64 ms, commit/16 7.9 → 2.5 ms, commit/128 29.4 → 16.8 ms, replay 144 → 123 µs. Cumulative: commit/1 2.5×, /16 3.4×, /128 1.7×, replay 1.8×; ingest/finish unchanged; 4 improved, 0 regressed.
Stack (8/13): base
perf/kr-07-positional-decided-row→ headperf/kr-08-alignment-reachable-load.Method. Numbers come from
crates/mc-module/benches/kernel_routes.rs(added in the first PR of this stack): a baseline binary and a candidate binary run as separate processes in 10 ABBA blocks pinned to one core; each cell ismedian_baseline / median_candidatewith a paired bootstrap 95% CI over the blocks. Baseline is the suite commit onstack/kernel-routes-03-routesbefore any optimization; ratios are therefore cumulative through this PR, and the marginal effect of this PR alone is called out. A/A control on the same host: all CIs include 1.0, half-widths about 1%. Host: AMD EPYC 9R14, SQLite 3.51.3 (bundled).Guard.
cargo test -p mc-module --features test-support --test kernel_routes(45),cargo test -p mc-kernel --features test-support(all suites),cargo clippy -p mc-module -p mc-kernel --all-targets -- -D warnings. No file undertests/changed; the wire contract, receipts, and serving semantics are unchanged.