fix(storage): route node-keyed families with node-only splitters, remove per-partition spill buffers (#1439) - #1440
Conversation
|
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 |
github-actions
Bot
added
core
|
Paused pending further investigation — do not merge. After this PR was opened, a closer read of the ladder evidence found that the gate's A diagnostic sweep (fixed 8.4M-edge fixture, |
This comment has been minimized.
This comment has been minimized.
…#1439) Past ~4.19M staged identity records, the cut formula `min(partition_count, max(1, total/16))` saturated at the flat `partition_count` cap (256 by default), so partition *count* stopped rising while partition *size* kept growing with data. Each partition is materialized whole in memory and sorted, so that let resident partition memory scale with data -- exactly what `rss_bounded_or_plateaued` exists to catch, and it refused S22. Add a second, data-driven term to the cut: floored at DEFAULT_PARTITION_COUNT (256) and growing as total_records / TARGET_ROWS_PER_PARTITION (16,384), min-combined with the existing small-scale floor (unchanged) and the requested ceiling. 16,384 is chosen so the term reproduces the already-proven 256-partition operating point at S18's record count and reaches exactly MAX_PARTITION_COUNT (4,096) at S22's, matching the table in #1439. Default `GraphConstructionBudgets::partition_count` to MAX_PARTITION_COUNT so this data-driven term, not an artificially low flat cap, is what bounds production partition counts; it remains a pure function of recorded staged data, never machine-derived (R1). Extends the cross-partition-count determinism test to the new default ceiling and adds a dedicated arithmetic test for the S18/S19/S20/S22 scaling table directly.
…ove per-partition spill buffers (#1439) Supersedes this branch's first commit: the data-driven partition-count scaling it introduced measured worse under a corrected fixture (RSS grew faster, not slower, once row-group buffer overhead was accounted for) and is reverted here back to main's `cut = min(partition_count, max(1, records/16))` with a flat 256 default. The real defect was elsewhere. Endpoints, node details and node-kind rows are keyed by node UUID, but were routed with splitters sampled from the *joint* node+edge identity domain. Graph500-shaped input puts nodes and edges in disjoint UUID bands (namespace byte) and is overwhelmingly edges, so almost every node UUID fell inside a handful of the joint splitters' partitions -- a real skew invisible to every existing check, because it was hidden by a second, independent defect: `edge_batch`/`edge_rows` test fixtures indexed nodes from a per-chunk-local counter, so every staged chunk referenced the same low node-UUID band regardless of graph size, and the whole test suite exercised a degenerate graph shape that never surfaced the skew. Fixes, in order of how they were found: - A second, node-only splitter set (`choose_node_partition_plan`, sampled from the staged node identity domain, recorded as `node_splitters` in `ShapeIntent`) routes node details, staged (pre-resolution) endpoints and node-kind rows. Edge-keyed families keep the joint set, whose domain they already match well. - `PartitionBalance::assert_balanced_with_hub_tolerance`: extends the balance assertion to every fixed-width family (previously only identities), discounting one repeated key's excess run before comparing to the tolerance -- a hub node's endpoint records genuinely cannot be split across partitions, so that is not a splitter defect. - Fixed the test-fixture bug: `edge_batch`/`edge_rows` now take an explicit node offset (and, where needed, a node count to wrap modulo), so src/dst references vary across chunks instead of collapsing onto the first chunk's nodes. - Fixed a partitioner-sizing bug found while wiring the above: node-keyed `FixedRangePartitioner`/`RowRangePartitioner` instances were sized from the joint plan's (larger) partition count while routing through the node plan (fewer partitions). `PartitionBalance`'s mean is `total/partitions`, so the unused trailing slots diluted the mean and made correctly-balanced data look skewed. Now sized from whichever plan actually routes them. - Removed the per-partition write buffer for the four fixed-width families (identities, node/edge details, endpoints): `SPILL_BLOCK_BYTES` (64 KiB) held open per partition per family was the dominant measured term in both RSS and fsync growth as partition count rises. `UnbufferedSpillWriter` holds no buffer; `PartitionRun` (in shape.rs) exploits the same monotone-sorted property the row partitioner already used, accumulating one contiguous same-partition run of wire bytes per staged chunk and writing it in a single call via the new `FixedRangePartitioner::route_slice`. Row groups (Arrow-based) are unchanged: `StreamWriter` doesn't expose per-record wire boundaries the same way, so the batching trick doesn't transfer, and left at a flat 256 the residual buffered term there cannot dominate. - Pre-sized `load_partition`'s `Vec` from the routing-time balance count, removing the growth-by-doubling headroom. Verified: cargo test -p graphforge-storage --lib (1,152 passed, 0 failed); the four fixed-width digests unchanged for shaped-identities.run and shaped-node-details.run (edge-details/edge-endpoints changed only because the fixture bug fix changes the graph structure the determinism tests build, not because partitioning changed); cross-partition-count fingerprint invariance holds at every requested count including the 4,096 ceiling; cargo clippy --workspace -- -D warnings clean. Measured at realistic (Graph500 UUID-band) ladder-rung scale, one ingest+shape per process, /usr/bin/time -v: peak RSS 78.2 / 96.2 / 138.2 MB at S18/S19/S20-equivalent record counts (edges/128 node ratio, matching the real ladder), versus 144.5 / 204.0 / 317.0 MB before the buffer removal at the same (corrected) ratio -- roughly halving both the absolute RSS and the growth rate. Still short of the ladder's own +13%/ +27% ingest growth, so the ladder itself is the next check, not this fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1955f17 to
dddf1e6
Compare
…omment (#1439) The header comment's historical measurement (on 13632d4, predating S5's external-merge-tree deletion) documents self-consistency across two sessions, not a pinned expectation -- no test asserts those literal values. Note that edge details and edge endpoints legitimately changed under the per-family splitter routing landed alongside this, while identities and node details did not, matching which families' routing actually changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DecisionNerd
changed the title
|
Ready for review. Summary of where this landed, since the branch went through several dead ends before this:
Not merging or enqueueing this myself. |
This comment has been minimized.
This comment has been minimized.
The balance check added in dddf1e6 refused every Graph500-shaped construction below ladder scale: the endpoints family is keyed by node and holds one record per incident edge, so with power-law degrees the partition that owns a few hubs is row-heavy by design, and discounting exactly one key cannot cover it (scale 10: largest partition 315 of 1,858 rows across 64, largest single-key run 91, mean 29). That is what CI's scale_g500_ladder target failed on. The splitters are quantiles of the key domain, so what they promise to balance is distinct keys per partition. finish_optional already walks each partition in key order; count key changes there and put that balance through the unchanged assert_balanced. For unique-key families it is the same number as before. A collapsed or mis-sampled splitter set still concentrates distinct keys and is still refused; a hub-heavy family over balanced keys is accepted. Both are pinned by fixtures. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…family #1440 removed the 64 KiB per-partition spill buffer from every fixed-width family and batched writes by contiguous same-partition run instead. Four of the five families arrive sorted by their routing key, so their runs are kilobytes each and the change was free. The Resolved family is re-keyed by edge UUID away from its node-UUID input order, so it has no runs at all: every resolved endpoint reached the descriptor as its own write(2), 33.6M calls for 840 MB at S20 (evidence: shape_consume_reauthentication.write_calls 93,118 -> 33,663,169 between 9269362 and 03013d0), and the shaping seal went from 113 s to 162 s. That is the ~23% ingest throughput regression the memory fix cost. Give the spill writer a per-family bound: zero for the four run-batched families, 8 KiB for Resolved, allocated lazily per open spill. Only the Resolved family holds spills open while it is live, so the residency is 256 partitions x 8 KiB = 2 MiB, against the 4 x 256 x 64 KiB #1440 removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit a39c4b832be3bff6fab0fddbde7ca044074ce625)
Measured after merge: the ~23% ingest regression is recovered, the S22 gate still admits (b0e53d3, #1443)The two commits folded into this PR at Cause of the regression, measured. It was not S18–S22 host ladder, quiet host (load 1.9→3.3), ingest phase:
Slope S18→S20: 1.80 B/edge (E0 1.81, main 4.86). Bound curve (quiet host, 4,194,304 edges, bench shape; Resolved per-partition bound → write submissions / VmHWM / edges/s / cpu µs per edge): 0 → 8,416,225 / 65.7 MB / 75,600 / 10.64; 8 KiB → 53,398 / 65.6 MB / 102,135 / 7.81; 16 KiB → 40,627 / 65.4 MB / 105,412 / 7.63; 64 KiB → 30,990 / 80.5 MB / 105,746 / 7.58. 8 KiB is the largest bound with no resident-memory cost (256 × 8 KiB, lazily allocated, only while the Resolved family is live); 64 KiB would add ~15–17 MB to ingest RSS for ~3.5% throughput. 256 KiB / 1 MiB were not run: the knob is per partition, so they fail the RSS property by arithmetic. Slope attribution (5.08 → 1.94 B/edge). Bench shape, 4.19M → 16.7M edges, VmHWM: node-plan splitters 65.7 → 89.3 MB (1.88 B/edge), same with all five families' 64 KiB buffers restored 87.7 → 100.1 (0.99); joint-plan splitters re-enabled (skew reproduced) 65.5 → 137.5 (5.72), same with buffers 69.4 → 136.0 (5.30). The per-family splitter fix is the slope; the buffers are a +4 to +22 MB intercept. Bounding the Resolved buffer costs nothing on the axis S22 admits on. Balance check. The one-hub discount refused every Graph500-shaped construction below ladder scale (scale 10: 315 of 1,858 rows across 64 partitions, mean 29). It now balances distinct keys per partition — the quantity the splitters are quantiles of — through the unchanged Verified: |
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>
1 participant
What this does
Fixes the mechanism behind #1439 (the S22 refusal on
rss_bounded_or_plateaued). The original diagnosis in #1439 (a flat partition-count cap making partition size grow with data) turned out not to be the dominant term once measured properly; this PR supersedes that approach entirely and fixes what the measurement actually pointed to.The defect: endpoints, node details and node-kind rows are keyed by node UUID, but were routed with splitters sampled from the joint node+edge identity domain. Graph500-shaped input puts nodes and edges in disjoint UUID bands (namespace byte) and is overwhelmingly edges, so almost every node UUID fell inside a handful of the joint splitters' partitions — a real skew that was invisible to every existing check, because it was hidden by a second, independent defect: the
edge_batch/edge_rowstest fixtures indexed nodes from a per-chunk-local counter, so every staged chunk referenced the same low node-UUID band regardless of graph size. The entire test suite was exercising a degenerate graph shape that never surfaced the skew. That fixture bug is arguably the more important find — it's a test-coverage defect that hid a production defect from the whole suite.The fix, in the order found:
choose_node_partition_plan, sampled from the staged node identity domain, recorded asnode_splittersinShapeIntent) routes node details, staged (pre-resolution) endpoints and node-kind rows. Edge-keyed families keep the joint splitter set, whose domain they already match well.PartitionBalance::assert_balanced_with_hub_toleranceextends the balance assertion to every fixed-width family (previously only identities), discounting one repeated key's excess run before comparing to the tolerance — a hub node's endpoint records genuinely cannot be split across partitions, so that's not a splitter defect.edge_batch/edge_rowsnow take an explicit node offset (and, where needed, a node count to wrap modulo), so src/dst references vary across chunks instead of collapsing onto the first chunk's nodes.FixedRangePartitioner/RowRangePartitionerinstances were sized from the joint plan's (larger) partition count while routing through the node plan (fewer partitions).PartitionBalance's mean istotal/partitions, so the unused trailing slots diluted the mean and made correctly-balanced data look skewed — a subtle, dangerous bug, since it would have produced spurious refusals that looked like real skew. Now sized from whichever plan actually routes them.SPILL_BLOCK_BYTES(64 KiB) held open per partition per family was the dominant measured term in both RSS and fsync growth as partition count rises.UnbufferedSpillWriterholds no buffer;PartitionRun(inshape.rs) exploits the same monotone-sorted property the row partitioner already used, accumulating one contiguous same-partition run of wire bytes per staged chunk and writing it in a single call via the newFixedRangePartitioner::route_slice. Row groups (Arrow-based) are unchanged —StreamWriterdoesn't expose per-record wire boundaries the same way, so the batching trick doesn't transfer, and this PR does not changeSPILL_BLOCK_BYTESor attempt to.load_partition'sVecfrom the routing-time balance count, removing the growth-by-doubling headroom.Explicitly reverted: an earlier commit on this branch made the partition count scale with data (
TARGET_ROWS_PER_PARTITION,MAX_PARTITION_COUNTas the new default). Measured properly, it made RSS growth worse, not better, once row-group buffer overhead was accounted for — raising the joint plan's partition count also raises the still-buffered row-group term. That's reverted back to main'scut = min(partition_count, max(1, records/16))with a flat 256 default. Buffer removal is the win; partition-count tuning was a dead end investigated and correctly abandoned.Verified
cargo test -p graphforge-storage --lib: 1,152-1,157 passed, 0 failed (one unrelated intermittent flake seen once inlifecycle_io::tests::qualification_rejects_an_unpaired_byte_only_row, from observe(storage): extend per-phase I/O attribution beyond the construction path #1422's new process-global counters, reproduced in isolation as passing — pre-existing, not caused by this change).cargo clippy --workspace -- -D warnings: clean.make pre-push-fast: clean (after rebasing onto the fix(ci): sort the import block in the migration ledger check #1438 lint fix on main).partition_count_changes_the_layout_but_not_the_logical_result) holds at every requested count including the 4,096 ceiling.shaped-identities.runandshaped-node-details.runare byte-identical before/after this change — their routing didn't change.shaped-edge-details.runandshaped-edge-endpoints.rundo change, and that's expected: routing node-keyed families with node-only splitters changes how the edge-keyed families partition too (different splitter boundaries → different partition contents, same final sorted output). This is not a determinism regression;unpinned_sessions_differ_only_in_the_wall_clock_runtime_catalogstill reports exactly one cross-session difference (the wall-clock runtime catalog). The determinism header comment inconstruction_determinism_tests.rsdocumented a historical measurement on a pre-perf(storage): range-partition shaping and delete the external merge tree #1430 commit (13632d4b) that happened to match two of these four names; it's flagged as stale for those two rows in a follow-up commit on this branch, since no test ever asserted those literal values.Measured: the real ladder, S18-S22, on this branch (
dddf1e65/03013d03)Built frozen binaries from this branch and ran the S18-S22 host ladder directly (couldn't use
gf-clean-ladder.sh— it refuses any SHA that isn't already an ancestor oforigin/main, which an open PR's branch isn't). Evidence at/home/ubuntu/graphforge-ladder/clean-1955f17d-evidence/.All four rungs passed, including S22:
S22's own admission (
s22-projection.json, sourced from S19/S20):rss_growth_fraction: 0.00717against a gate of<= 0.10— every check true,decision: "admitted". S20's admission (sourced from S18/S19):rss_growth_fraction: 0.0029. Rung-wide RSS is flat across the whole 16x edge range (182.2 → 196.3 MB, 4.6% of the 4 GiB budget). This is the gate that refused S22 in #1439, and it now passes with more than 10x margin.The cost: throughput regression, same rungs, same host, vs. the prior ladder on
9269362f9269362f)S20 wall time went from ~319s to ~381s. My read: removing the per-partition buffer traded write batching for memory — each partition now gets one
write_allper contiguous chunk-slice instead of buffering into 64 KiB blocks, so more, smaller syscalls. I haven't verified that mechanism directly (e.g. with strace), so treat it as the leading hypothesis, not a confirmed cause. Extrapolated (not measured), S26 would move from ~5.5h to ~7.9h against a 3.20h ceiling. Not asked to fix this — the RSS gate was the assignment — but stating it plainly so it's on the record rather than rediscovered at S24.The one-rung caveat
Ingest RSS still grows appreciably S19→S20 (97.3 → 113.6 MB, ~+16.8%) even after this fix — the rung-wide max barely moves (0.7%) only because ingest currently sits below the nearly-flat
reopen_proofphase ceiling (~181-184 MB across S18-S20). Projecting forward, ingest crosses back overreopen_proofaround S22, and S24 admission (sourced from S20+S22) would likely see growth in the ~54% range and refuse. This buys one rung, not the whole ladder. It's a real, worthwhile fix — S22 was the immediate blocker — but it is not a general fix for RSS growth at scale, and whoever picks up S24 should not be surprised.Separately: the S20→S22 wall-time exponent (1.114) is higher than S18→S20's (1.022), but this is not attributable to this branch — the prior engine (
9269362f) never ran S22, so there's no same-engine comparison at that scale to hold this branch to. Two-point exponent fits without a same-engine baseline have already produced one phantom "superlinearity" finding on this effort; flagging this explicitly so it doesn't become a second one.On provenance of this ladder run
I rebased this worktree (
9269362fbase →origin/main, picking up #1438 and #1422) while the S18-S22 run was in progress, reasoning that the run used frozen Rust binaries built before the rebase. That's true for the binaries, but the harness's Python (benchmarks/harness) is read live, not frozen, and the rebase did touch two harness files (progressive_run.py,lifecycle_runtime.py) that are imported by the rung assembly path — so S18 ran against the pre-rebase harness and S19-S22 likely ran against the post-rebase one. I checked the actual diff: the change is purely additive new per-phase lifecycle I/O attribution (_lifecycle_application_io), gated on Rust-side receipt fields that only exist in binaries built after #1422 — this branch's binaries predate that, so the new code path returnsNoneand is a no-op for every rung here.progressive_qualification.pyandprogressive_host_run.py(which actually computerss_growth_fractionand the admission decision) were untouched by the rebase. So I believe the result stands, but the mixed provenance is real and I'm recording it rather than asserting a clean run I can't fully back. A from-scratch re-run against a fully stable worktree (no rebase mid-run) would remove the last bit of doubt; I attempted one but the host was contended by unrelated activity for the remainder of this session.Not done in this PR
StreamWriter) still hold one buffered spill per partition; not touched, per the reasoning above.Do not merge or enqueue — reporting the PR URL per process; this is ready for review.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Note
Route node-keyed families with node-only splitters and unbuffered spill writes
ShapeIntentand validated byvalidate_shape_binding.UnbufferedSpillWriter, which writes directly to the hashing writer and finalizes each spill without retaining a per-partition block buffer.FixedRangePartitioner::route_sliceand the newPartitionRunhelper batch contiguous same-partition records into one write, andload_partitionpreallocates from the recorded row count.PartitionBalance::assert_balanced_with_hub_toleranceallows excess rows from one repeated key but rejects excess concentration of distinct keys;RowRangePartitioneroutput now applies a standard balance assertion after row assembly.edge_batch,edge_rows, lifecycle and codec tests) now generate endpoint references across the full node domain using a node-start and node-count parameter.shape_canonical_with_cancellationpersists two splitter sets inShapeIntent; shape manifests and checkpoints written by older versions will not carry thenode_only_splittersfield (serde-defaulted to empty), andvalidate_shape_bindingnow checks the node-only splitter authority before binding.Macroscope summarized 03013d0.