perf(kernel): read every candidate's object state in one statement and each digest's facts once - #208
Conversation
…ment and each digest's egress facts once egress_candidates ran load_object_state per candidate and egress_facts_tx per candidate even when candidates shared a digest, so a 1024-candidate batch executed over two thousand statements after the served-rows query.
|
Warning Review limit reachedNext included review available in 8 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 108 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 (3)
Comment |
|
Reviewed the diff. This is a clean, well-scoped perf change — batching Checked closely and didn't find correctness issues:
Minor/non-blocking note: the benchmark added in the stack's first PR exercises many candidates sharing one digest (the scenario this PR optimizes for), but the correctness test suite doesn't have a case with several candidates sharing the same I wasn't able to run |
…e and batch readers load_object_state and load_object_states carried the same SELECT list, joins, and column positions; one helper now builds the SQL and one mapper reads the row, so the token-check and egress paths cannot drift apart.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // One registry read for the batch, and one egress-facts read per | ||
| // distinct digest, instead of one of each per candidate. | ||
| let states = load_object_states(tx, &ids)?; | ||
| let mut artifacts: HashMap<&str, ArtifactEgressFacts> = HashMap::new(); |
There was a problem hiding this comment.
The correctness of this batch depends on two dedup assumptions holding for every caller: (1) load_object_states collapsing duplicate object_ids to a single state is safe because o.object_id is unique in object_registry, and (2) memoizing ArtifactEgressFacts by digest is safe because destination is fixed for the whole call. Both look right from reading the schema/signature, but I don't see a test at this level (or in kernel_routes) that exercises a batch with a repeated object_id or repeated digest and asserts the per-candidate results still line up correctly with their index. Given this is exactly the case the PR is optimizing for (candidates sharing a digest), it'd be worth a small regression test here so a future change to either dedup path fails loudly instead of only showing up as a perf regression.
There was a problem hiding this comment.
Fixed in 1ddb997. Confirmed the gap: the only batch test (eligibility_verdicts_cover_every_class_and_cache_per_incarnation_and_tip) uses 11 distinct ids and a single digest, so neither dedup path was exercised.
Added egress_candidates_line_up_by_index_when_ids_and_digests_repeat in crates/mc-kernel/tests/kernel_cas.rs. It builds a 7-candidate batch with a repeated object id (naming two different digests), a repeated digest (named by a live object and by a never-written id), a repeated unknown digest, and a None digest, then for both destinations asserts each batch entry equals a single-candidate egress_candidates read at the same index, plus that repeated ids/digests share one answer and distinct digests do not.
Sanity-checked the oracle by mutating the memo to return any cached facts regardless of digest; the test fails on the batch[1].artifact == batch[2].artifact / assert_ne!(batch[0].artifact, batch[1].artifact) pair.
|
Reviewed the diff (admission.rs + envelope.rs). Summary: Solid, well-scoped perf change.
Minor, non-blocking: left one inline note — I didn't find a test that exercises a batch with a repeated No security or behavioral issues found; changes are internal to |
Resolve the conflict in load_object_state: main hoisted its query text into a LazyLock, while this branch factored the text into object_state_sql shared with load_object_states. Keep the shared helper and build each variant once behind a LazyLock so both readers avoid the per-call format! allocation.
Review summaryThis is a clean, well-scoped perf change. I traced the logic in
One minor nitpick left inline (non-blocking) about avoiding an extra Nice, thorough benchmarking methodology in the description (paired bootstrap CI, A/A control). No security concerns — no new untrusted-input paths, and the batched query still uses parameter binding throughout. |
| let states = statement | ||
| .query_map([ids], object_state_from) | ||
| .map_err(map_sqlite)? | ||
| .collect::<rusqlite::Result<Vec<_>>>() | ||
| .map_err(map_sqlite)?; | ||
| Ok(states | ||
| .into_iter() | ||
| .map(|state| (state.object.object_id.clone(), state)) | ||
| .collect()) |
There was a problem hiding this comment.
Minor: this collects into an intermediate Vec<ObjectState> and then rebuilds a HashMap from it, which allocates twice for a batch that can be 1024 items. Since the goal of this PR is cutting per-batch overhead, collecting directly into the map avoids the extra pass:
| let states = statement | |
| .query_map([ids], object_state_from) | |
| .map_err(map_sqlite)? | |
| .collect::<rusqlite::Result<Vec<_>>>() | |
| .map_err(map_sqlite)?; | |
| Ok(states | |
| .into_iter() | |
| .map(|state| (state.object.object_id.clone(), state)) | |
| .collect()) | |
| statement | |
| .query_map([ids], object_state_from) | |
| .map_err(map_sqlite)? | |
| .map(|row| row.map(|state| (state.object.object_id.clone(), state))) | |
| .collect::<rusqlite::Result<HashMap<_, _>>>() | |
| .map_err(map_sqlite) |
Non-blocking — the savings are small relative to the statement-count win this PR already delivers.
There was a problem hiding this comment.
Fixed in 14ba722. One note on the suggestion as written: returning the chain directly as the tail expression fails to compile (statement does not live long enough — the Rows temporary outlives the statement), so the result is bound to a local first and then returned. Same single-pass collect into HashMap, no intermediate Vec.
…d digests repeat egress_candidates now reads every object's state in one statement and each digest's egress facts once. Nothing exercised a batch that repeats an object id or a digest, so a change to either dedup path would only show up as a perf regression. Compare each batch entry with a single-candidate read at the same index, and check that repeated ids and digests share one answer while distinct digests keep distinct facts.
|
Follow-up on the two review passes:
|
load_object_states collected every row into a Vec and then rebuilt a HashMap from it, two allocations for a batch that can hold 1024 states. Collect into the map in one pass.
|
Reviewed the diff (admission.rs, envelope.rs, kernel_cas.rs). No correctness, security, or performance issues found. Notes from the review:
One very minor, non-blocking observation: |
|
Re the duplicate-ids observation: agreed it's harmless, and leaving it as-is on purpose. Deduping before serializing would add a set allocation and a hash pass on every call to shave a few bytes off the bound JSON parameter, and the new test pins the semantics either way. Not changing it unless a workload shows high intra-batch duplication. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by gemini-3.8-flash · Input: 185.8K · Output: 24.3K · Cached: 1.2M |
1 participant
egress_candidatesranload_object_stateper candidate andegress_facts_txper candidate even when candidates shared a digest, so a 1024-candidate batch executed over two thousand statements after the served-rows query.load_object_statesreads the whole id list with onejson_eachquery (duplicate candidate ids get the same state, as before), and egress facts are memoized per distinct digest within the batch.Measured (marginal): eligibility cold 1 77 → 69 µs, 64 695 → 640 µs, 1024 10.9 → 10.2 ms; warm unchanged. Cumulative cold 4.6×/3.5×/3.3×; 6 improved, 0 regressed.
Stack (11/13): base
perf/kr-10-page-single-hop→ headperf/kr-11-batch-egress-candidates.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.