perf(#5954): derive the directed adjacency once as a CSR and share it (slices 5+6) - #6025
Merged
Merged
Conversation
… (slices 5+6) After the S4 scratch-reuse fix (#6017, -675 MB on the real corpus), profiling attributed 105.1 MB — 62% of what remained — to `g.Edges()` boxing ~1.33M edge values into interfaces. The cause was that the graph's adjacency STRUCTURE was re-derived from the gonum graph up to four times per Pass-4 run, each derivation going through an interface-returning iterator that materialises one boxed graph.WeightedEdge per edge into a slice sized by the whole edge set. This derives the directed adjacency ONCE, in BuildGraph, straight from the relationship slice (so it costs no boxing at all), and has both consumers read it: sampled betweenness and the Louvain undirected projection. WHAT CHANGED * `directedCSR` (algorithms.go): n+1 int32 offsets, flat int32 neighbours, parallel float64 weights. Node i is gonum node id i — BuildGraph hands out ids 0..n-1 — so no consumer needs an id translation. int32 is sized against the reference corpus (427k entities / 1.33M collapsed edges, both ~1000x below MaxInt32) and halves the index footprint vs int64. Allocation is strictly count-then-fill: pass 1 counts raw out-degrees, pass 2 fills exactly-sized arrays, parallel edges collapse in place, and the result is copied into exactly-sized final arrays. No append-from-zero. Every field is written once during construction and is read-only thereafter — the two derivation counters live on nodeIndex so this object has no writer at all once BuildGraph returns, which is what makes the aliasing below safe. * BuildGraph builds the CSR FIRST and materialises the gonum edge set from it, rather than walking the relationship slice twice. Relationship.PropsSnapshot() materialises a fresh map[string]string per call, so the two-walk ordering read every relationship's properties twice; see MEASURED below for what that cost. Building in this order also spares gonum the lookup/remove/re-add chain that parallel edges used to trigger, because the CSR arrives pre-collapsed. * `newBCScratch` ALIASES the shared CSR (it already had exactly the right shape) instead of walking g.Edges(). The gonum-derived fallback is kept for hand-built test graphs whose id space is not 0..n-1, and is pinned to produce byte-identical adjacency. * `buildCSRFromDirected` (louvain.go) projects the directed CSR straight into Louvain's csrGraph. This deletes the intermediate simple.WeightedUndirectedGraph that ComputeCommunities used to build purely so buildCSRFromUndirected could walk it back out again — a map-of-maps copy of the whole edge set plus two more boxed edge iterations. * ComputeCommunities' modularity sweep reads the undirected CSR instead of gonum's edge iterator, and its per-node stats are three flat slices instead of a ~430k-entry map[int64]*nodeStat. * ComputeCommunities no longer takes the gonum graph (it does not use it). The DIRECTED gonum graph itself is UNCHANGED in content and still built: PageRank (ComputeCentrality) and IdentifyArticulationPoints both consume it. MEASURED Fixture: buildHeavyTailedGraph(60000, 4, 5) — preferential-attachment (Barabasi-Albert), heavy in-degree tail, one cluster holding ~26% of entities; ~239k relationships collapsing to ~205k directed edges. The generator emits no parallel edges, self-loops or dangling endpoints; those three BuildGraph behaviours are pinned by tests, not by this benchmark. The reference corpus is ~6.5x more edges, so these are per-edge proxies, NOT a prediction of corpus RSS. The corpus saving is unmeasured. Whole pass, main vs HEAD, same machine, back to back, props-carrying fixture: RunAlgorithmsWithOptions main HEAD ---------------------------------------------------------------- wall 1150 ms/op 857 ms/op -25% churn 499.3 MB/op 335.9 MB/op -33% allocations 3,286,779/op 2,390,629/op -27% peak HeapInuse 229.6 / 241.7 MB 190.1 / 191.6 MB Isolated consumers (allocation figures; deterministic, machine-independent): sampledBetweenness (K=512) 34.11 MB/op 313 a/op -> 8.17 MB/op 278 a/op ComputeCommunities 177.71 MB/op 1,004,964 -> 36.62 MB/op 108,848 BuildGraph is ~3.6 MB/op heavier: that is the CSR, retained rather than transient. RELATIONSHIP-PROPERTY COST — the reason BuildGraph builds the CSR first. Same fixture, same machine, 3x10 runs each, ms/op: main two-walk HEAD rels WITH properties 130.8 156.9 119.6 rels WITHOUT 115.3 127.5 105.6 The middle column is the first version of this change, which walked the relationship slice twice and so called PropsSnapshot() twice per edge: +20% on the props fixture. Corpus relationships carry callsite_count/confidence on essentially every edge. Allocation was unaffected (Go stack-allocates the small non-escaping maps), so nothing above is undermined — but wall time is an explicit requirement on this epic, and re-inflating relationship properties is precisely what slice 1 (#5976) existed to stop. Building the CSR first removes the second read and lands BuildGraph ~8% FASTER than main on both fixtures. buildHeavyTailedGraph emits relationships with NO PROPERTIES, so PropsSnapshot() returns nil and edgeWeight short-circuits: the regression was invisible on it. BenchmarkBuildGraph_WithProps and csrBenchFixtureWithProps are added so the next change to edge-weight derivation cannot hide the same way. NOT DONE IdentifyArticulationPoints still walks g.Edges() once. It is the last remaining boxed edge iteration on the pass and the obvious next slice; it was left alone here to keep this change to the two consumers the issue names. EQUIVALENCE Output is bit-identical, not approximately equal: * legacyBuildGraph and legacyComputeCommunities (algorithms_csr_legacy_test.go) are verbatim copies of the pre-change implementations, kept as INDEPENDENT oracles. This matters more than usual now: production derives the gonum edge set from the CSR, so checking one against the other would be checking a representation against itself. * TestDirectedCSRMatchesGonumAdjacencyBitForBit checks BOTH the CSR and the gonum graph against legacyBuildGraph, node by node, weights included, on fixtures carrying isolated entities, self-loops, parallel edges, dangling and blank endpoints. * The three BuildGraph behaviours are ALSO pinned independently (TestDirectedCSRKeepsIsolatedEntities / DropsSelfLoops / CollapsesParallelEdgesWithAccumulatedWeight). * TestDirectedCSRHighFanOutRow covers a 60k-out-degree hub with interleaved parallel edges — the only shape that stresses the maxRow-sized scratch buffers, the (target, arrival) key packing at large arrival indices, and the hasEdge/weightOf binary searches over a row deep enough to recurse. * TestUndirectedProjectionFromCSRMatchesGonumProjection compares the whole csrGraph (off/adj/w/selfw/k/m2) against the gonum route it replaces. * TestComputeCommunitiesMatchesLegacyGonumPath compares the entire returned tuple against the legacy implementation, with modularity asserted to FULL PRECISION (math.Float64bits), not a tolerance. * TestRunAlgorithmsEndToEndUnchanged does the same through the public entry point, including betweenness against the legacy gonum-walking oracle. * The parallel-edge collapse preserves ARRIVAL order (the sort key packs (target, position-in-row) into one int64), because BuildGraph's `w += existing.Weight()` chain is order-dependent. Reversing that order is mutation M9 below and is caught. * `(sigma[p]/sigma[w]) * (1+delta[w])` is untouched; hoisting the division is mutation M7 and is caught. One float-order caveat, stated plainly: the modularity sweep previously summed weighted degrees in gonum's map-derived edge order, which differs run to run. It now sums in ascending (u,v) order — strictly MORE deterministic, but a raw sum may land on a different last ulp than any one prior run happened to produce. roundForDeterminism quantises the published value far above that, and the golden tests assert the published values bit-for-bit over repeated legacy runs. GUARDS * TestDirectedCSRIsBuiltExactlyOncePerBuildGraph asserts idx.csrBuilds == 1 and idx.undirectedDerivations == 1 across a full pass (BuildGraph + ComputeCommunities + ComputeCentrality + IdentifyArticulationPoints). * TestSampledBetweennessUsesSharedCSROnTheBuildGraphPath asserts the scratch actually ALIASES the shared arrays rather than quietly taking the fallback — a construction counter alone would not catch a consumer that re-derives. * TestDirectedCSRIndexLimitIsEnforced drives BuildGraph with csrIndexLimit lowered and requires a panic on BOTH the node and the edge bound. csrIndexLimit is a var solely so this test can exercise the real path: a bound only reachable by calling a helper directly is a bound nothing proves is wired up. The previous revision checked only the node count while claiming to check both, and degraded to an all-zero CSR — which newBCScratch accepts, yielding all-zero betweenness and n singleton communities, persisted as if they were scores. It now panics. MUTATION TESTING Every new test was mutation-tested; 20 defects introduced, 20 caught: M1 self-loops not dropped -> DropsSelfLoops, MatchesGonumAdjacency M2 parallel edges not collapsed -> CollapsesParallelEdges, MatchesGonumAdjacency M3 trailing isolated entities dropped -> MatchesGonumAdjacency M4 directed CSR built twice -> IsBuiltExactlyOnce M5 undirected projection derived twice -> IsBuiltExactlyOnce M6 betweenness falls back to walking gonum -> UsesSharedCSR M7 Brandes division hoisted -> SampledBetweenness_BitIdenticalToLegacy M8 reciprocal weights not summed -> UndirectedProjection, CommunitiesMatchesLegacy M9 collapse loses arrival order -> MatchesGonumAdjacency, CollapsesParallelEdges M10 undirected degree double-counts pairs -> UndirectedProjection M11 isolated node's row offset corrupted -> KeepsIsolatedEntities M12a modularity 2m halved -> CommunitiesMatchesLegacy, RunAlgorithmsEndToEnd M12b degree credited to one endpoint -> CommunitiesMatchesLegacy, RunAlgorithmsEndToEnd M13 empty-graph CSR without offsets -> EmptyAndEdgeless M14 gonum edge weight from wrong CSR slot -> MatchesGonumAdjacency, HighFanOutRow M15 last edge of every gonum row dropped -> MatchesGonumAdjacency, HighFanOutRow M16 row scratch sized by the wrong bound -> HighFanOutRow M17 int32 overflow guard removed -> IndexLimitIsEnforced M18 derivation counter no longer bumped -> IsBuiltExactlyOnce M9 initially escaped TestDirectedCSRCollapsesParallelEdgesWithAccumulatedWeight and TestDirectedCSRHighFanOutRow: both compared the CSR against the graph BuildGraph returned, which is now derived FROM the CSR. Both now compare against legacyBuildGraph, and both kill it. Two mutations were dropped as EQUIVALENT rather than uncaught: changing the sweep filter `v <= u` to `v < u` (self-loops cannot occur in the undirected CSR), and removing the filter entirely (uniformly doubling k, 2m, internalW and degree leaves q and the degree ranking algebraically unchanged). TEST ADAPTERS louvainPartition, louvainPartitionWithSweeps and buildCSRFromUndirected now have no production caller and survive only as the gonum-graph adapter the Louvain QUALITY suite runs on. That is sound because TestUndirectedProjectionFromCSRMatchesGonumProjection proves the two projections bit-identical — and that single test is the only bridge between the quality suite and production. Both functions now say so. DRIVE-BY FIX (#6024) TestComputeCentrality_ByteIdenticalOnSampledPath has been FAILING on main since #5954 made the betweenness map sparse: it built its expectation by pre-seeding a zero for every entity and then asserted key-count equality against a map that only holds non-zero scores (4000 vs 852). Fixed to build the expectation the way ComputeCentrality builds the real thing (algorithms.go:983-989). The red-main window is tracked as #6024. Ideally this would have been a preceding standalone commit rather than folded in with the change it guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The directed adjacency is now derived once, as a compact CSR (int32 offsets + int32 neighbours + float64 weights), built directly from the relationship slice in
BuildGraph.sampledBetweennessand the in-repo Louvain both consume it instead of walking the gonum graph and boxing ~1.33M edges into interfaces.The directed gonum graph stays — PageRank and
IdentifyArticulationPointsstill use it. It is now materialised from the CSR rather than alongside it, which is what makes this a wall-time win as well as a memory one (see below).Deleting the intermediate
simple.WeightedUndirectedGraph— which existed only to be walked straight back out into Louvain's own CSR — turned out to be a bigger win than the shared CSR itself.Measured
Fixture
buildHeavyTailedGraph(60000, 4, 5), ~236k relationships / ~205k edges. The reference corpus is ~6.5x more edges, so the corpus-scale saving is unmeasured and deliberately not extrapolated — prior estimates in this epic were wrong three times, in both directions.ComputeCommunitiessampledBetweenness(K=512)The regression that the benchmark could not see
Review caught this and it is the most useful thing in the PR. The first version computed each relationship's
edgeWeight(rels[i].PropsSnapshot())twice — once inBuildGraph's own loop, once in the CSR pass.PropsSnapshot()builds a freshmap[string]stringeach call.That cost was invisible in the benchmark, because
csrBenchFixturebuilds relationships with no properties at all, soPropsSnapshot()returned nil andedgeWeightshort-circuited. The real corpus carriescallsite_count/confidenceon essentially every edge. A fixture that structurally cannot exhibit the cost of the thing being measured is worse than no benchmark.Measured, 3x10 runs back-to-back,
BuildGraphms/op:Fixed by inverting the order rather than threading a weight slice through (which would have cost a
len(rels)-sized float slice, ~15 MB at corpus scale, in a memory-reduction epic): build the CSR first, materialise the gonum edge set from it. One property read per relationship, and gonum also loses its lookup/remove/re-add chain for parallel edges because the CSR arrives pre-collapsed.BenchmarkBuildGraph_WithPropsandcsrBenchFixtureWithPropsare added, andcsrBenchFixture's doc now states outright that it cannot exhibit property cost.That restructure made two tests tautological — with
gderived from the CSR,TestDirectedCSRCollapsesParallelEdgesWithAccumulatedWeightandTestDirectedCSRHighFanOutRowwere comparing the CSR against itself. This was caught because mutation M9 stopped killing them. Both now compare against a verbatimlegacyBuildGraphoracle, andTestDirectedCSRMatchesGonumAdjacencyBitForBitchecks the CSR andgagainst it. M9 kills again.Equivalence
BuildGraph's three load-bearing behaviours are reproduced exactly and pinned individually: a node for every entity including isolated ones, self-loops dropped, parallel edges collapsed. The collapse packs(target, arrival)into an int64 sort key so the float accumulation order matches gonum'sw += existing.Weight()chain.Numerical equivalence is asserted, not assumed.
TestSampledBetweenness_BitIdenticalToLegacypins the raw values — it is what catches hoisting(sigma[p]/sigma[w]) * (1+delta[w]), which is algebraically equal but not bit-identical.TestComputeCommunitiesMatchesLegacyGonumPathdeep-equals the entity→community map, community sizes,TopEntities, the denoise count, and per-community modularity bit-for-bit across 4 fixtures x 3 legacy repetitions. Community membership is structurally immune to the sum reordering: the partition comes fromlouvainPartitionFromCSR(ucsr), anducsris asserted bit-identical to the legacy projection.20 mutations introduced, 20 caught. Two further candidates were dropped as genuinely equivalent rather than uncaught, with the algebra shown:
v <= u→v < uin the modularity sweep (buildCSRFromDirectedcannot emitv == u), and removing that filter entirely (uniform doubling ofI,Kandm2leavesqinvariant and preserves the degree ordering).Hardening from review
n, never the edge count, while the doc claimed both — and the degraded path returned a structurally valid, all-zero-offset CSR thatnewBCScratchaccepted, yielding all-zero betweenness and n singleton communities. A silent wrong answer, persisted as scores. Both bounds are now enforced (edge count accumulated inint64so the check cannot itself wrap) and it panics.csrIndexLimitbecame avaronly so the test can lower it and drive the realBuildGraph— a bound reachable only through a helper is the "guard that does not bind the path that runs" this codebase keeps producing. Independently verified: disabling the edge-count check failsTestDirectedCSRIndexLimitIsEnforced/edge_count.directedCSRnow has no writer at all afterBuildGraphreturns; theundirectedDerivationscounter moved tonodeIndex. That makes the invariant justifyingnewBCScratch's aliasing literal rather than "read-only except one field".buildCSRFromUndirectedandlouvainPartitionWithSweepsnow have no production caller and survive as test adapters, so the entire Louvain quality suite runs against the adapter. That is sound becauseTestUndirectedProjectionFromCSRMatchesGonumProjectionproves the two projections bit-identical — and both functions now carry a comment naming that test as the single bridge, and stating that weakening it silently disconnects the quality suite from production.Also fixes the red main
TestComputeCentrality_ByteIdenticalOnSampledPathhas failed onmainsince it was introduced by #6017 — its legacy oracle pre-seeded a zero per entity while #6011 had deliberately made the centrality map sparse (legacy=4000 new=852). The expectation is now a literal mirror of the production loop atalgorithms.go:983-989, fixing the key set while leaving values compared bit-for-bit viamath.Float64bits.This should have been a preceding standalone commit — an author repairing the oracle that guards their own change is a pattern a reviewer should never have to take on trust. It was instead verified independently: by provenance, by matching the expectation to production, and by three mutations (perturbed value, dropped pivot, corrupted shared
adj) that each kill the repaired test.Verification
go build ./... && go vet ./... && gofmt -l .clean.internal/graph428 pass;-race -count=2556 pass. Full repo exit 0, 228 packages, zero failures — confirmed viartk proxy go testrather than the rtk summary line, which truncates above 10 MiB and reports a green summary over a red run.Known limits
Corpus-scale saving unmeasured. Equivalence fixtures top out at 3000 entities apart from the 60k hub. The int32 bound is enforced but tested only via a lowered limit, never at true scale.
Remaining known boxing:
IdentifyArticulationPointsstill walksg.Edges()once — the last boxed edge iteration in the pass, and the obvious next slice.Independently adversarially reviewed.
Closes #6024
Refs #5954