perf(storage,api): remove three per-record hot loops from validate (-33% user CPU) - #1458
Conversation
Measured at S18 on the #1440 head (two runs each, shaped-output digests byte-identical to main in every run): main 27.3 user-s A: 23.6 A+B: 20.9 A+B+C: 18.2 (-9.2 s, -33% user) A. write_bounded_row_groups computed each row's byte contribution with column.slice(i, 1).to_data().get_slice_memory_size(): four heap allocations per row per column, 52M of the 104M allocations validate made at S18 (heaptrack). RowBytes computes the same number from the column's layout; anything outside the enumerated fast paths still takes the exact arrow call, and a unit test pins the two equal for every fast path with and without a null buffer. B. DetailCodec::bytes scanned the 250 padding bytes of every in-memory 272/304-byte detail record on every routing, sort and encode pass, and read() re-ran it on the record it had just zeroed itself. wire() returns the length-bounded prefix; the UTF-8 and non-empty checks stay on read. C. Edge normalization built three BTreeSet<Uuid> per batch (candidate endpoints, candidate edges, observed) and one per node batch: 6.7% of validate's CPU in BTreeMap::insert. Candidate sets are now sorted, deduplicated Vecs (same order the BTreeSet iterated, so index probes are unchanged) and membership sets are HashSets. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
CI's `cargo clippy --workspace -- -D warnings` rejected `read`: removing the `self.bytes(&record)` call took away its only use of the receiver, since the record it validates is one it zero-initialised itself. The receiver is kept so `read` stays symmetric with `bytes` and `wire`, and so a second codec variant needs no call-site churn -- a targeted allow with that reason recorded, not a signature change. `wire` keeps its `let Self::Compact = self` and needs no allow; an earlier attempt put one there, which suppressed nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5a74506 to
e254250
Compare
Summary
Three per-record hot loops removed from
import-session validate— −33 % user CPU at S18 with shaped-output digests byte-identical to main in every run.This attacks the constraint everything else routes around.
validateis a single-threaded, CPU-bound pipeline at 0.85-0.89 cores — 132.2 CPU-seconds at S20 (S18 measured 26.56 user + 6.95 sys = 33.51, x3.945). Every parallelism attempt so far (#1429 at ~1.34x, #1448 measured at 1.46-1.58x against a 2.0 Amdahl ceiling) has been bounded by that floor. Removing work lowers it directly.The three loops
A.
write_bounded_row_groupscomputed each row's byte contribution withcolumn.slice(i, 1).to_data().get_slice_memory_size()— four heap allocations per row per column, and 52M of the 104M allocations validate made at S18 (heaptrack).RowBytescomputes the same number from the column's layout. Anything outside the enumerated fast paths still takes the exact arrow call, and a unit test pins the two equal for every fast path, with and without a null buffer.B.
DetailCodec::bytesscanned the 250 padding bytes of every in-memory 272/304-byte detail record on every routing, sort and encode pass, andread()re-ran it on the record it had just zeroed itself.wire()returns the length-bounded prefix; the UTF-8 and non-empty checks stay onread.C. Edge normalization built three
BTreeSet<Uuid>per batch (candidate endpoints, candidate edges, observed) plus one per node batch — 6.7 % of validate's CPU inBTreeMap::insert. Candidate sets are now sorted, deduplicatedVecs (the same order theBTreeSetiterated, so index probes are unchanged); membership sets areHashSets.On B, since it removes a validation
Every producer of a padded detail record zero-initialises it, so the padding scan verified an invariant construction already guarantees. Verified at all five construction sites:
graph_construction_encoding.rs:2853,intake.rs:456and:477,shape.rs:978,uuid_membership/rebuild.rs:833— each[0_u8; N], withread()filling onlyprefix + 1 + lengthbytes into a fresh buffer.The wire path still validates.
bytes()is unchanged and still does the full check on wire-bearing bytes; itsis_errtests atconstruction_detail_codec.rs:202-208still target it.No coverage regression. A corrupted length byte that truncates a name was not caught by the padding check either —
read()only ever wrotelengthbytes, so the padding stayed zero in that case too. The only thing the scan could catch is memory corruption between construction and use, which ADR 0013's threat model excludes and nothing else in the codebase defends against.Provenance and what still needs checking
This branch was written by an agent that hit a session limit before reporting. The work was committed and measured; I preserved the branch rather than let it be lost with the worktree, and I did not re-run its gates locally — a timed S24-S26 ladder is running on the host and a local build would contaminate its wall times.
So CI is the verification here. Specifically watch:
cargo test -p graphforge-api --test bdd— TCK 3,897permanent_storage_budgetsandscale_g500_laddercargo clippy --workspace -- -D warningsIf any of those fail this should not merge on a re-run without diagnosis. The measured numbers above are the agent's, taken at S18 on the #1440 head; they have not been reproduced at S20 on a quiet host and should be treated as measured-at-S18, projected elsewhere.
Part of #1456. Related #1441 (compact records, which B partly addresses), #1387.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.