From de61b879b4005877fc8a62f9d2f46b1408033b95 Mon Sep 17 00:00:00 2001 From: Jorge Cajas Date: Tue, 28 Jul 2026 08:28:07 +0800 Subject: [PATCH] perf(#5954): derive the directed adjacency once as a CSR and share it (slices 5+6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft --- internal/graph/algorithms.go | 493 +++++++++--- .../graph/algorithms_betweenness_mem_test.go | 46 +- internal/graph/algorithms_csr_bench_test.go | 131 ++++ internal/graph/algorithms_csr_legacy_test.go | 243 ++++++ internal/graph/algorithms_csr_test.go | 714 ++++++++++++++++++ internal/graph/algorithms_sampled_test.go | 10 +- internal/graph/louvain.go | 129 ++++ 7 files changed, 1635 insertions(+), 131 deletions(-) create mode 100644 internal/graph/algorithms_csr_bench_test.go create mode 100644 internal/graph/algorithms_csr_legacy_test.go create mode 100644 internal/graph/algorithms_csr_test.go diff --git a/internal/graph/algorithms.go b/internal/graph/algorithms.go index 8f62f7707..c0ea51ff2 100644 --- a/internal/graph/algorithms.go +++ b/internal/graph/algorithms.go @@ -139,6 +139,26 @@ type nodeIndex struct { toInt map[string]int64 fromInt map[int64]string next int64 + + // csr is the directed adjacency of the graph BuildGraph just built, in + // compact CSR form. It is derived ONCE, from the relationship slice, and + // then shared by every consumer that needs adjacency (sampled betweenness, + // the Louvain undirected projection). See directedCSR. + csr *directedCSR + // csrBuilds counts how many times a directed CSR was derived for this + // index. It must be exactly 1 after BuildGraph and must not grow when the + // downstream algorithms run — that invariant is the entire point of #5954 + // S5/S6 and is pinned by TestDirectedCSRIsBuiltExactlyOncePerBuildGraph. + csrBuilds int + // undirectedDerivations counts how many undirected projections have been + // derived from csr. One full Pass-4 run must derive exactly one; a second + // would mean the Louvain path is walking the structure twice again. + // + // It lives here rather than on directedCSR so that directedCSR itself has + // NO writer once BuildGraph returns: newBCScratch ALIASES its arrays, and + // "read-only apart from one counter" is a weaker invariant than that + // aliasing deserves. + undirectedDerivations int } func newNodeIndex() *nodeIndex { @@ -159,12 +179,274 @@ func (n *nodeIndex) get(id string) int64 { return v } +// directedCSR is the directed adjacency of the graph BuildGraph constructs, +// in compressed-sparse-row form: `off` holds n+1 row offsets and `adj`/`w` are +// the flat, row-major neighbour and weight arrays. Node i of the CSR is gonum +// node id i (BuildGraph hands out ids 0..n-1 in entity order), so no id +// translation is needed by any consumer. +// +// # Why this exists (#5954 S5/S6) +// +// The directed gonum graph stays — ComputeCentrality's PageRank and +// IdentifyArticulationPoints both consume it. But the adjacency STRUCTURE was +// previously re-derived from it up to four separate times per Pass-4 run, each +// time through gonum's interface-returning edge iterator, which materialises +// one boxed graph.WeightedEdge per edge into a slice sized by the whole edge +// set. Profiling after the S4 scratch-reuse fix attributed 105.1 MB (62% of +// what was left) to exactly that boxing. Deriving the adjacency once, from the +// relationship slice, and sharing it removes those walks outright. +// +// # int32 sizing +// +// The reference corpus is ~427k entities and ~1.33M collapsed edges, both three +// orders of magnitude below math.MaxInt32, so int32 indices are safe and halve +// the footprint of `off`/`adj` relative to int64. buildDirectedCSR ENFORCES the +// bound on both the node count and the pre-collapse edge count — see +// csrIndexLimit for why it enforces by panicking. +// +// # Mutability +// +// Every field is written once, during construction, and is READ-ONLY +// thereafter. newBCScratch relies on that: it aliases off/adj rather than +// copying them. Do not add mutable state here — the two derivation counters +// this change needs live on nodeIndex precisely so this object has no writer +// after BuildGraph returns. +type directedCSR struct { + n int + off []int32 // len n+1 + adj []int32 // len off[n], target node ids, ASCENDING within a row + w []float64 // len off[n], parallel to adj +} + +// csrIndexLimit is the largest node count and edge count directedCSR's int32 +// indices can address. +// +// Exceeding it PANICS rather than degrading. The alternative — returning a +// truncated or all-zero CSR — is a silent wrong answer: newBCScratch would +// accept an all-zero offset array and produce all-zero betweenness, and the +// Louvain projection would produce n singleton communities, both of which are +// then PERSISTED as if they were real scores. A graph two thousand times larger +// than the reference corpus is a situation that needs a human, not a fallback. +// +// It is a var only so TestDirectedCSRIndexLimitIsEnforced can lower it and drive +// the real BuildGraph path: a bound that can only be exercised by calling a +// helper directly is a bound nothing proves is wired into the code that runs. +// Nothing in production writes it. +var csrIndexLimit int64 = math.MaxInt32 + +// row returns the [lo, hi) slice bounds of u's adjacency row. +func (c *directedCSR) row(u int32) (int32, int32) { return c.off[u], c.off[u+1] } + +// hasEdge reports whether the directed edge u->v is present. Rows are sorted +// ascending, so this is a binary search. +func (c *directedCSR) hasEdge(u, v int32) bool { + lo, hi := c.row(u) + for lo < hi { + mid := lo + (hi-lo)/2 + switch { + case c.adj[mid] < v: + lo = mid + 1 + case c.adj[mid] > v: + hi = mid + default: + return true + } + } + return false +} + +// weightOf returns the weight of u->v and whether it exists. +func (c *directedCSR) weightOf(u, v int32) (float64, bool) { + lo, hi := c.row(u) + for lo < hi { + mid := lo + (hi-lo)/2 + switch { + case c.adj[mid] < v: + lo = mid + 1 + case c.adj[mid] > v: + hi = mid + default: + return c.w[mid], true + } + } + return 0, false +} + +// csrEndpoints applies BuildGraph's edge-admission rules to one relationship +// and returns the dense endpoint pair. It is the SINGLE definition of "which +// relationships become edges", shared with BuildGraph's gonum insertion loop so +// the two cannot drift: +// +// - blank endpoint id -> rejected +// - endpoint not an entity -> rejected (bare stdlib names etc.) +// - self-loop -> rejected (gonum rejects them on simple graphs) +// +// Parallel edges are ADMITTED here and collapsed downstream, mirroring +// BuildGraph's weight accumulation. +func csrEndpoints(idx *nodeIndex, r *Relationship) (from, to int32, ok bool) { + if r.FromID == "" || r.ToID == "" { + return 0, 0, false + } + f, ok := idx.toInt[r.FromID] + if !ok { + return 0, 0, false + } + t, ok := idx.toInt[r.ToID] + if !ok { + return 0, 0, false + } + if f == t { + return 0, 0, false + } + return int32(f), int32(t), true +} + +// buildDirectedCSR derives the compact directed adjacency straight from the +// relationship slice — it never touches the gonum graph, so it costs no edge +// boxing at all. +// +// It reproduces BuildGraph's three structural behaviours exactly: +// +// 1. every entity is a node, including isolated ones (n = idx.next, so a node +// with no incident relationship still gets an — empty — row); +// 2. self-loops are dropped (csrEndpoints); +// 3. parallel edges are collapsed with their weights ACCUMULATED, in +// relationship order, so the resulting float is bit-identical to the +// `w += existing.Weight()` chain BuildGraph runs. +// +// Allocation is count-then-fill, never append-from-zero: pass 1 counts raw +// out-degrees, pass 2 fills exactly-sized raw arrays, then parallel edges are +// collapsed in place and the result copied into exactly-sized final arrays. +func buildDirectedCSR(idx *nodeIndex, rels []Relationship) *directedCSR { + idx.csrBuilds++ + n := int(idx.next) + if int64(n) > csrIndexLimit { + panic(fmt.Sprintf("graph: %d entities exceeds the int32 CSR index limit %d", n, csrIndexLimit)) + } + c := &directedCSR{n: n, off: make([]int32, n+1)} + if n == 0 { + return c + } + + // Pass 1 — raw out-degree per source. Parallel edges are still counted + // separately at this point; they collapse below. The running total is int64 + // so the bound check below cannot itself be the thing that overflows. + rawOff := make([]int32, n+1) + var maxRow int32 + var admitted int64 + for i := range rels { + u, _, ok := csrEndpoints(idx, &rels[i]) + if !ok { + continue + } + rawOff[u+1]++ + admitted++ + } + if admitted > csrIndexLimit { + panic(fmt.Sprintf("graph: %d admitted relationships exceeds the int32 CSR index limit %d", admitted, csrIndexLimit)) + } + for i := 0; i < n; i++ { + if d := rawOff[i+1]; d > maxRow { + maxRow = d + } + rawOff[i+1] += rawOff[i] + } + rawTotal := rawOff[n] + + // Pass 2 — fill. Within a row, entries land in relationship order, which + // is what makes the collapse below reproduce BuildGraph's accumulation + // order. + adj := make([]int32, rawTotal) + w := make([]float64, rawTotal) + cursor := make([]int32, n) + copy(cursor, rawOff[:n]) + for i := range rels { + u, v, ok := csrEndpoints(idx, &rels[i]) + if !ok { + continue + } + p := cursor[u] + adj[p] = v + w[p] = edgeWeight(rels[i].PropsSnapshot()) + cursor[u] = p + 1 + } + + // Collapse parallel edges in place. Rows are processed in ascending order + // and the write cursor never overtakes the row start, so the compacted + // prefix is always behind the row being read. `rowW` shadows the row's + // weights because the write can land on the row's own first slot. + // + // The sort key packs (target, arrival index within the row) into one int64 + // so slices.Sort — no closure, no reflect swapper — gives a target-ordered, + // arrival-stable permutation. Arrival stability is what preserves the + // float accumulation order. + keys := make([]int64, 0, maxRow) + rowW := make([]float64, maxRow) + var write int32 + for u := 0; u < n; u++ { + lo, hi := rawOff[u], rawOff[u+1] + c.off[u] = write + if lo == hi { + continue + } + keys = keys[:0] + for p := lo; p < hi; p++ { + keys = append(keys, int64(adj[p])<<32|int64(p-lo)) + } + copy(rowW, w[lo:hi]) + slices.Sort(keys) + for j := 0; j < len(keys); { + target := int32(keys[j] >> 32) + // BuildGraph computes `w := edgeWeight(...)` then `w += existing`, + // i.e. new + accumulated. IEEE-754 addition is commutative, so the + // left-to-right accumulation here is bit-identical. + sum := rowW[keys[j]&0xffffffff] + j++ + for j < len(keys) && int32(keys[j]>>32) == target { + sum += rowW[keys[j]&0xffffffff] + j++ + } + adj[write] = target + w[write] = sum + write++ + } + } + c.off[n] = write + + if write == rawTotal { + c.adj, c.w = adj, w + return c + } + c.adj = make([]int32, write) + c.w = make([]float64, write) + copy(c.adj, adj[:write]) + copy(c.w, w[:write]) + return c +} + // BuildGraph constructs a weighted directed graph plus an index mapping // string entity IDs to gonum int64 node IDs. Edge weight follows the spec: // // weight = max(1, callsite_count) * confidence // // with both properties drawn from Relationship.Properties (string-typed). +// +// The CSR is built FIRST and the gonum edge set is then materialised from it +// (#5954 S5/S6). The obvious ordering — build g by walking rels, then derive the +// CSR by walking rels again — reads every relationship's properties twice, and +// Relationship.PropsSnapshot() materialises a fresh map[string]string on every +// call. On a 60k-entity fixture whose relationships actually carry +// callsite_count/confidence (as corpus relationships do) that cost 31.6% of +// BuildGraph's wall time; it is invisible on a fixture with no properties, +// which is how it initially escaped review. Building in this order reads them +// exactly once, and additionally spares gonum the remove-and-reinsert churn +// that parallel edges used to cause, because the CSR arrives pre-collapsed. +// +// The two representations are therefore no longer independent derivations. +// legacyBuildGraph in algorithms_csr_legacy_test.go preserves the original +// walk-rels-into-gonum implementation verbatim, and +// TestDirectedCSRMatchesGonumAdjacencyBitForBit checks BOTH the CSR and g +// against it, so the equivalence claim still rests on an independent oracle. func BuildGraph(entities []Entity, rels []Relationship) (*simple.WeightedDirectedGraph, *nodeIndex) { g := simple.NewWeightedDirectedGraph(0, 0) idx := newNodeIndex() @@ -177,31 +459,20 @@ func BuildGraph(entities []Entity, rels []Relationship) (*simple.WeightedDirecte } } - for _, r := range rels { - if r.FromID == "" || r.ToID == "" { - continue - } - // Skip edges whose endpoints aren't in the entity set (e.g. bare - // stdlib names): they'd inflate node count without contributing - // real structure. - if _, ok := idx.toInt[r.FromID]; !ok { - continue - } - if _, ok := idx.toInt[r.ToID]; !ok { - continue - } - from := idx.get(r.FromID) - to := idx.get(r.ToID) - if from == to { - continue // gonum rejects self-loops on simple graphs - } - w := edgeWeight(r.PropsSnapshot()) - // If the edge already exists, accumulate weight (multiple call sites). - if existing := g.WeightedEdge(from, to); existing != nil { - w += existing.Weight() - g.RemoveEdge(from, to) + // Derive the shared directed adjacency ONCE. Every downstream consumer of + // adjacency reads this instead of re-walking g's interface-returning edge + // iterator. See directedCSR. + idx.csr = buildDirectedCSR(idx, rels) + + // Materialise the gonum edge set from the CSR. Self-loops are already gone + // and parallel edges already collapsed, so this is one SetWeightedEdge per + // surviving edge with no lookup, no RemoveEdge and no re-accumulation. + for u := int32(0); u < int32(idx.csr.n); u++ { + lo, hi := idx.csr.row(u) + for p := lo; p < hi; p++ { + g.SetWeightedEdge(g.NewWeightedEdge( + simple.Node(int64(u)), simple.Node(int64(idx.csr.adj[p])), idx.csr.w[p])) } - g.SetWeightedEdge(g.NewWeightedEdge(simple.Node(from), simple.Node(to), w)) } return g, idx } @@ -378,39 +649,21 @@ func atofSafe(s string) float64 { // result slice and their members are assigned community_id=-1 ("ungrouped"). // This prevents singleton/micro-community noise from reaching the MCP surface // and the dashboard. Set opts.MinSize=1 to disable denoising. -func ComputeCommunities(g *simple.WeightedDirectedGraph, idx *nodeIndex, entityNames []string, opts CommunityOptions) ([]CommunityResult, map[string]int, float64, int) { - // Project the directed graph onto an undirected graph; community detection - // in gonum operates on undirected (or otherwise symmetric) inputs. - und := simple.NewWeightedUndirectedGraph(0, 0) - nodes := g.Nodes() - for nodes.Next() { - n := nodes.Node() - if und.Node(n.ID()) == nil { - und.AddNode(simple.Node(n.ID())) - } - } - edges := g.WeightedEdges() - for edges.Next() { - e := edges.WeightedEdge() - from, to := e.From().ID(), e.To().ID() - if from == to { - continue - } - if existing := und.WeightedEdge(from, to); existing != nil { - w := existing.Weight() + e.Weight() - und.RemoveEdge(from, to) - und.SetWeightedEdge(und.NewWeightedEdge(simple.Node(from), simple.Node(to), w)) - continue - } - und.SetWeightedEdge(und.NewWeightedEdge(simple.Node(from), simple.Node(to), e.Weight())) - } +func ComputeCommunities(idx *nodeIndex, entityNames []string, opts CommunityOptions) ([]CommunityResult, map[string]int, float64, int) { + // Undirected projection, derived ONCE from the shared directed CSR + // BuildGraph already built (#5954 S5/S6). This replaced a + // simple.WeightedUndirectedGraph that existed only to be walked straight + // back out again by buildCSRFromUndirected: two boxed edge iterations plus + // a full map-of-maps copy of the edge set, for a projection that is one + // pass over flat arrays. + ucsr, ids := buildCSRFromDirected(idx) // In-repo Louvain (louvain.go). No PRNG: node order, adjacency order and // tie-breaks are all fixed, so the partition is deterministic by // construction rather than by seeding. See #5954 / louvain.go for why // gonum's community.Modularize was replaced. const resolution = 1.0 - groups := louvainPartition(und, resolution) + groups, _ := louvainPartitionFromCSR(ucsr, ids, resolution) // Issue #633 phase-2 — pprof showed `community.Q` accounted for ~90% of // indexing allocations (21.6 GB on client-fixture-b: 9,549 communities × @@ -426,53 +679,52 @@ func ComputeCommunities(g *simple.WeightedDirectedGraph, idx *nodeIndex, entityN // and gonum's "2*w_uv for u u` + // filter visits it exactly once, in ascending (u, v) order. The previous + // version walked gonum's edge iterator, whose order is map-derived and + // therefore differed run to run — so this loop is strictly MORE + // deterministic than what it replaces, at the cost that a float sum here + // may land on a different last ulp than any one prior run happened to + // produce. roundForDeterminism() quantises the published value well above + // that. internalW := make([]float64, len(groups)) var m2 float64 - wedges := und.WeightedEdges() - for wedges.Next() { - e := wedges.WeightedEdge() - w := e.Weight() - fid, tid := e.From().ID(), e.To().ID() - nf, ok := nodeStats[fid] - if !ok { - // Node absent from the partition: louvainPartition covers every - // node of the undirected projection, but defensively guard so the - // loop is total. - nf = &nodeStat{cidIdx: -1} - nodeStats[fid] = nf - } - nt, ok := nodeStats[tid] - if !ok { - nt = &nodeStat{cidIdx: -1} - nodeStats[tid] = nt - } - nf.k += w - nt.k += w - nf.degree++ - nt.degree++ - m2 += 2 * w // undirected: each edge contributes 2 to Σ k. - if nf.cidIdx >= 0 && nf.cidIdx == nt.cidIdx { - internalW[nf.cidIdx] += w + for u := int32(0); u < int32(n); u++ { + for p := ucsr.off[u]; p < ucsr.off[u+1]; p++ { + v := ucsr.adj[p] + if v <= u { + continue + } + w := ucsr.w[p] + nodeK[u] += w + nodeK[v] += w + nodeDeg[u]++ + nodeDeg[v]++ + m2 += 2 * w // undirected: each edge contributes 2 to Σ k. + if nodeCID[u] >= 0 && nodeCID[u] == nodeCID[v] { + internalW[nodeCID[u]] += w + } } } @@ -481,9 +733,7 @@ func ComputeCommunities(g *simple.WeightedDirectedGraph, idx *nodeIndex, entityN for cid, gg := range groups { var k float64 for _, nid := range gg { - if ns, ok := nodeStats[nid]; ok { - k += ns.k - } + k += nodeK[nid] } K[cid] = k } @@ -520,11 +770,7 @@ func ComputeCommunities(g *simple.WeightedDirectedGraph, idx *nodeIndex, entityN members := make([]member, 0, len(g)) for _, nid := range g { communityOf[idx.fromInt[nid]] = cid - deg := 0 - if ns, ok := nodeStats[nid]; ok { - deg = ns.degree - } - members = append(members, member{nid, deg}) + members = append(members, member{nid, int(nodeDeg[nid])}) } // Issue #481 — degree ties were resolved by map-iteration order // (g.Nodes / und.From); tiebreak on the gonum int64 node id so @@ -748,7 +994,7 @@ func ComputeCentrality(g *simple.WeightedDirectedGraph, idx *nodeIndex) (map[str // Large group union (#5349 A4 / #5692): exact Brandes is O(V·E) and the // enrichment-bound cost (~240s on a 291k-node graph). Use the // deterministic sampled-pivot approximation. - raw = sampledBetweenness(g, betweennessSampleSize, betweennessSampleSeed) + raw = sampledBetweenness(g, idx.csr, betweennessSampleSize, betweennessSampleSeed) case betweennessPathExactWeighted: // FloydWarshall is O(V^3) and precomputes all shortest paths; on // graphs <= cutoff this is the most accurate option. @@ -817,7 +1063,11 @@ func ComputeCentrality(g *simple.WeightedDirectedGraph, idx *nodeIndex) (map[str // across runs of the same graph (the on-disk determinism contract, #481). // Unweighted shortest paths are used (matching network.Betweenness, the exact // fallback above the FloydWarshall cutoff) so the comparison is apples-to-apples. -func sampledBetweenness(g *simple.WeightedDirectedGraph, k int, seed uint64) map[int64]float64 { +// csr is the shared directed adjacency from BuildGraph; when it covers this +// graph the BFS reads it directly and no edge iteration happens at all. Pass +// nil for a hand-built graph (tests), which falls back to deriving adjacency +// from g's edge iterator. +func sampledBetweenness(g *simple.WeightedDirectedGraph, csr *directedCSR, k int, seed uint64) map[int64]float64 { nodes := gonumgraph.NodesOf(g.Nodes()) v := len(nodes) cb := make(map[int64]float64, v) @@ -850,7 +1100,7 @@ func sampledBetweenness(g *simple.WeightedDirectedGraph, k int, seed uint64) map // ascending-id `ids` slice, so adjacency sorted by dense index is the same // order as the previous sort-by-node-id — every float summation below // therefore happens in exactly the order the map-based version used. - sc := newBCScratch(g, ids) + sc := newBCScratch(g, ids, csr) for _, s := range pivots { sc.accumulatePivot(sc.dense.of(s), cb, ids) @@ -951,16 +1201,28 @@ type bcScratch struct { // dense maps gonum node ids to dense indices; retained so callers can // translate a pivot id without rebuilding the mapping. dense *denseIDMap + + // sharedCSR records whether off/adj alias the directedCSR BuildGraph + // derived (true) or were re-derived here from g's edge iterator (false). + // Production must always be true; the false branch exists only for + // hand-built test graphs. Asserted by + // TestSampledBetweennessUsesSharedCSROnTheBuildGraphPath. + sharedCSR bool } // newBCScratch builds the dense successor CSR and allocates all per-run scratch. // ids must be ascending; dense index i corresponds to gonum node ids[i]. -func newBCScratch(g *simple.WeightedDirectedGraph, ids []int64) *bcScratch { +// csr, when non-nil and dimensionally compatible with ids, is used DIRECTLY as +// the successor adjacency: it already has exactly the shape this needs (row +// offsets plus ascending-sorted targets, dense index == gonum node id), so the +// slices are aliased rather than copied and g's edge iterator is never touched. +// That is the #5954 S5/S6 win — the 1.33M-edge boxing walk this function used +// to perform is gone on the production path. +func newBCScratch(g *simple.WeightedDirectedGraph, ids []int64, csr *directedCSR) *bcScratch { n := len(ids) sc := &bcScratch{ n: n, dense: newDenseIDMap(ids), - off: make([]int32, n+1), dist: make([]int32, n), gen: make([]uint32, n), sigma: make([]float64, n), @@ -969,12 +1231,31 @@ func newBCScratch(g *simple.WeightedDirectedGraph, ids []int64) *bcScratch { predHead: make([]int32, n), } if n == 0 { + sc.off = make([]int32, 1) + return sc + } + + // The shared CSR is addressed by gonum node id, so it is usable only when + // this graph's id space is exactly 0..n-1 — which is what BuildGraph hands + // out. Any other id space (hand-built test graphs) takes the fallback. + if csr != nil && csr.n == n && sc.dense.contiguous && sc.dense.base == 0 { + sc.sharedCSR = true + sc.off = csr.off + sc.adj = csr.adj + total := csr.off[n] + sc.predNext = make([]int32, total) + sc.predNode = make([]int32, total) + for i := range sc.predHead { + sc.predHead[i] = -1 + } return sc } - // Materialise the edge list ONCE via a single global iterator. Calling - // g.From(id) per node instead would allocate one gonum iterator per node - // per pass — 2V allocations and hundreds of MB of churn at corpus scale. + sc.off = make([]int32, n+1) + // Fallback: materialise the edge list ONCE via a single global iterator. + // Calling g.From(id) per node instead would allocate one gonum iterator per + // node per pass — 2V allocations and hundreds of MB of churn at corpus + // scale. dense := sc.dense type edge struct{ u, v int32 } var edges []edge @@ -1518,7 +1799,7 @@ func RunAlgorithmsWithOptions(entities []Entity, rels []Relationship, opts Commu // 6.6 MB at 433k entities). names := nodeNames(entities, idx) - commResults, commOf, overallQ, denoised := ComputeCommunities(g, idx, names, opts) + commResults, commOf, overallQ, denoised := ComputeCommunities(idx, names, opts) // Layer-1 deterministic naming (TF-IDF over member entity names + // qualified names + source-file basenames). Mutates commResults in place. AssignCommunityNames(commResults, entities, commOf) diff --git a/internal/graph/algorithms_betweenness_mem_test.go b/internal/graph/algorithms_betweenness_mem_test.go index 521350332..fe1221d26 100644 --- a/internal/graph/algorithms_betweenness_mem_test.go +++ b/internal/graph/algorithms_betweenness_mem_test.go @@ -206,9 +206,9 @@ func TestSampledBetweenness_BitIdenticalToLegacy(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { ents, rels := buildHeavyTailedGraph(tc.n, tc.deg, tc.seed) - g, _ := BuildGraph(ents, rels) + g, idx := BuildGraph(ents, rels) want := sampledBetweennessLegacy(g, tc.k, betweennessSampleSeed) - got := sampledBetweenness(g, tc.k, betweennessSampleSeed) + got := sampledBetweenness(g, idx.csr, tc.k, betweennessSampleSeed) assertSameBetweenness(t, want, got) nonzero := 0 for _, v := range got { @@ -225,7 +225,7 @@ func TestSampledBetweenness_BitIdenticalToLegacy(t *testing.T) { // empty graph, and a graph with no edges at all (every BFS visits one node). func TestSampledBetweenness_EmptyAndIsolated(t *testing.T) { empty := simple.NewWeightedDirectedGraph(0, 0) - if got := sampledBetweenness(empty, 8, betweennessSampleSeed); len(got) != 0 { + if got := sampledBetweenness(empty, nil, 8, betweennessSampleSeed); len(got) != 0 { t.Fatalf("empty graph: want 0 entries, got %d", len(got)) } @@ -233,9 +233,9 @@ func TestSampledBetweenness_EmptyAndIsolated(t *testing.T) { for i := range ents { ents[i] = Entity{ID: fmt.Sprintf("e%d", i), Name: fmt.Sprintf("E%d", i), Kind: "function"} } - g, _ := BuildGraph(ents, nil) + g, idx := BuildGraph(ents, nil) want := sampledBetweennessLegacy(g, 8, betweennessSampleSeed) - got := sampledBetweenness(g, 8, betweennessSampleSeed) + got := sampledBetweenness(g, idx.csr, 8, betweennessSampleSeed) assertSameBetweenness(t, want, got) } @@ -247,12 +247,18 @@ func TestComputeCentrality_ByteIdenticalOnSampledPath(t *testing.T) { g, idx := BuildGraph(ents, rels) legacyRaw := sampledBetweennessLegacy(g, betweennessSampleSize, betweennessSampleSeed) - want := make(map[string]float64, len(ents)) - for _, id := range idx.toInt { - want[idx.fromInt[id]] = 0 - } + // ComputeCentrality's betweenness map is SPARSE: #5954 stopped storing a + // rounded score of zero so that "absent" is the single representation of a + // zero betweenness. This expectation used to be built by pre-seeding a zero + // for every entity, which made the key-count assertion below compare 4000 + // pre-seeded keys against the ~850 the production map actually holds — the + // test has been failing on main since that change. Build it the same way + // ComputeCentrality does instead. + want := make(map[string]float64, len(legacyRaw)) for nid, v := range legacyRaw { - want[idx.fromInt[nid]] = roundForDeterminism(sanitizeFloat(v)) + if rv := roundForDeterminism(sanitizeFloat(v)); rv != 0 { + want[idx.fromInt[nid]] = rv + } } got, _ := ComputeCentrality(g, idx) @@ -283,12 +289,12 @@ func TestComputeCentrality_ByteIdenticalOnSampledPath(t *testing.T) { // would expose scratch left dirty between calls). func TestSampledBetweenness_Deterministic(t *testing.T) { ents, rels := buildHeavyTailedGraph(2000, 4, 21) - g, _ := BuildGraph(ents, rels) - a := sampledBetweenness(g, 128, betweennessSampleSeed) + g, idx := BuildGraph(ents, rels) + a := sampledBetweenness(g, idx.csr, 128, betweennessSampleSeed) other, orels := buildHeavyTailedGraph(500, 3, 22) - og, _ := BuildGraph(other, orels) - _ = sampledBetweenness(og, 64, betweennessSampleSeed) - b := sampledBetweenness(g, 128, betweennessSampleSeed) + og, oidx := BuildGraph(other, orels) + _ = sampledBetweenness(og, oidx.csr, 64, betweennessSampleSeed) + b := sampledBetweenness(g, idx.csr, 128, betweennessSampleSeed) assertSameBetweenness(t, a, b) } @@ -355,16 +361,16 @@ func TestSampledBetweenness_ScratchIsReusedAcrossPivots(t *testing.T) { } const n = 60_000 ents, rels := buildHeavyTailedGraph(n, 5, 0x5954) - g, _ := BuildGraph(ents, rels) + g, idx := BuildGraph(ents, rels) const kLo, kHi = 64, 512 var out map[int64]float64 _, churnLo := betweennessMemProbe(func() { - out = sampledBetweenness(g, kLo, betweennessSampleSeed) + out = sampledBetweenness(g, idx.csr, kLo, betweennessSampleSeed) }) runtime.KeepAlive(out) peakHeap, churnHi := betweennessMemProbe(func() { - out = sampledBetweenness(g, kHi, betweennessSampleSeed) + out = sampledBetweenness(g, idx.csr, kHi, betweennessSampleSeed) }) runtime.KeepAlive(out) runtime.KeepAlive(g) @@ -402,7 +408,7 @@ func TestSampledBetweenness_ScratchIsReusedAcrossPivots(t *testing.T) { func BenchmarkSampledBetweenness_HeavyTailed(b *testing.B) { const n = 60_000 ents, rels := buildHeavyTailedGraph(n, 5, 0x5954) - g, _ := BuildGraph(ents, rels) + g, idx := BuildGraph(ents, rels) var peakSum, iters uint64 b.ReportAllocs() @@ -410,7 +416,7 @@ func BenchmarkSampledBetweenness_HeavyTailed(b *testing.B) { for i := 0; i < b.N; i++ { var out map[int64]float64 p, _ := betweennessMemProbe(func() { - out = sampledBetweenness(g, betweennessSampleSize, betweennessSampleSeed) + out = sampledBetweenness(g, idx.csr, betweennessSampleSize, betweennessSampleSeed) }) runtime.KeepAlive(out) peakSum += p diff --git a/internal/graph/algorithms_csr_bench_test.go b/internal/graph/algorithms_csr_bench_test.go new file mode 100644 index 000000000..10c299a8d --- /dev/null +++ b/internal/graph/algorithms_csr_bench_test.go @@ -0,0 +1,131 @@ +package graph + +import ( + "fmt" + "runtime" + "testing" +) + +// csrBenchFixture is the shape every CSR benchmark in this file runs on. +// +// buildHeavyTailedGraph is preferential-attachment (Barabasi-Albert): heavy +// in-degree tail, one cluster holding ~26% of entities. It is the closest +// synthetic shape to the reference corpus that this package already has. It +// does NOT contain parallel edges, self-loops or dangling endpoints (the +// generator de-dups); those three BuildGraph behaviours are pinned by the +// equivalence tests in algorithms_csr_test.go instead, not by this benchmark. +// +// n=60000 / degree 4 gives ~240k relationships. The reference corpus is 427k +// entities / 1.33M collapsed edges, i.e. ~5.5x larger; numbers here are a +// per-edge proxy, not an absolute prediction of corpus RSS. +// +// IMPORTANT: buildHeavyTailedGraph emits relationships with NO PROPERTIES, so +// PropsSnapshot() returns nil and edgeWeight short-circuits. Corpus +// relationships carry callsite_count/confidence on essentially every edge, and +// a version of this change that read those properties twice cost 31.6% of +// BuildGraph's wall time while being completely invisible on this fixture. +// Anything that touches edge-weight derivation must be benchmarked on +// csrBenchFixtureWithProps as well — see BenchmarkBuildGraph_WithProps. +func csrBenchFixture() ([]Entity, []Relationship) { + return buildHeavyTailedGraph(60000, 4, 5) +} + +// csrBenchFixtureWithProps is csrBenchFixture with callsite_count/confidence +// attached to every relationship, which is what the real corpus looks like. +// Same topology, so the two are directly comparable and the difference between +// them is exactly the cost of reading relationship properties. +func csrBenchFixtureWithProps() ([]Entity, []Relationship) { + ents, rels := csrBenchFixture() + withProps := make([]Relationship, len(rels)) + for i := range rels { + withProps[i] = rels[i].WithProperties(map[string]string{ + "callsite_count": fmt.Sprintf("%d", 1+i%5), + "confidence": fmt.Sprintf("0.%02d", 11+i%80), + }) + } + return ents, withProps +} + +func BenchmarkBuildGraph_HeavyTailed(b *testing.B) { + ents, rels := csrBenchFixture() + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + g, idx := BuildGraph(ents, rels) + if g == nil || idx == nil { + b.Fatal("nil graph") + } + } +} + +// BenchmarkBuildGraph_WithProps is the same topology with corpus-shaped +// relationship properties. It exists because Relationship.PropsSnapshot() +// materialises a fresh map per call, so any implementation that reads a +// relationship's properties more than once shows up HERE and nowhere else. +func BenchmarkBuildGraph_WithProps(b *testing.B) { + ents, rels := csrBenchFixtureWithProps() + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + g, idx := BuildGraph(ents, rels) + if g == nil || idx == nil { + b.Fatal("nil graph") + } + } +} + +// BenchmarkSampledBetweenness_SharedCSR isolates the betweenness consumer. +// Before #5954 S5/S6 this function derived its own adjacency by walking g's +// interface-returning edge iterator; now it aliases the shared CSR. +func BenchmarkSampledBetweenness_SharedCSR(b *testing.B) { + ents, rels := csrBenchFixture() + g, idx := BuildGraph(ents, rels) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = sampledBetweenness(g, idx.csr, betweennessSampleSize, betweennessSampleSeed) + } +} + +// BenchmarkComputeCommunities_SharedCSR isolates the Louvain consumer. Before +// #5954 S5/S6 this built a whole simple.WeightedUndirectedGraph and then walked +// it back out twice; now it projects the shared CSR in one pass. +func BenchmarkComputeCommunities_SharedCSR(b *testing.B) { + ents, rels := csrBenchFixture() + _, idx := BuildGraph(ents, rels) + names := nodeNames(ents, idx) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _, _, _ = ComputeCommunities(idx, names, DefaultCommunityOptions()) + } +} + +// BenchmarkPass4_HeavyTailed measures the whole pass-4 sweep through the public +// entry point, and reports peak HeapInuse over baseline alongside the standard +// churn numbers — peak heap, not TotalAlloc, is what decides whether the +// group-algo child process swaps. +func BenchmarkPass4_HeavyTailed(b *testing.B) { + b.Setenv("GRAFEL_BETWEENNESS_SAMPLE_THRESHOLD", "100") + ents, rels := csrBenchFixture() + + var peakSum, iters uint64 + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + var res *AlgorithmResults + p, _ := betweennessMemProbe(func() { + res = RunAlgorithmsWithOptions(ents, rels, DefaultCommunityOptions()) + }) + if res == nil { + b.Fatal("nil result") + } + runtime.KeepAlive(res) + peakSum += p + iters++ + } + b.StopTimer() + if iters > 0 { + b.ReportMetric(float64(peakSum/iters)/(1<<20), "peakHeapInuseMB") + } +} diff --git a/internal/graph/algorithms_csr_legacy_test.go b/internal/graph/algorithms_csr_legacy_test.go new file mode 100644 index 000000000..8462f8ab3 --- /dev/null +++ b/internal/graph/algorithms_csr_legacy_test.go @@ -0,0 +1,243 @@ +package graph + +import ( + "sort" + + "gonum.org/v1/gonum/graph/simple" +) + +// legacyBuildGraph is a VERBATIM copy of BuildGraph's edge loop as it stood +// immediately before #5954 S5/S6: walk the relationship slice, apply the +// admission rules inline, and accumulate parallel edges into the gonum graph +// with a lookup/remove/re-add chain. +// +// It is the independent oracle for directedCSR. Production no longer derives +// the gonum edge set and the CSR separately — the CSR is built first and g is +// materialised from it — so without this copy the "CSR matches gonum" claim +// would be checking a representation against itself. +// +// Do NOT tidy this function. Its entire value is that it is unchanged. +func legacyBuildGraph(entities []Entity, rels []Relationship) (*simple.WeightedDirectedGraph, *nodeIndex) { + g := simple.NewWeightedDirectedGraph(0, 0) + idx := newNodeIndex() + + for _, e := range entities { + nid := idx.get(e.ID) + if g.Node(nid) == nil { + g.AddNode(simple.Node(nid)) + } + } + + for _, r := range rels { + if r.FromID == "" || r.ToID == "" { + continue + } + if _, ok := idx.toInt[r.FromID]; !ok { + continue + } + if _, ok := idx.toInt[r.ToID]; !ok { + continue + } + from := idx.get(r.FromID) + to := idx.get(r.ToID) + if from == to { + continue // gonum rejects self-loops on simple graphs + } + w := edgeWeight(r.PropsSnapshot()) + if existing := g.WeightedEdge(from, to); existing != nil { + w += existing.Weight() + g.RemoveEdge(from, to) + } + g.SetWeightedEdge(g.NewWeightedEdge(simple.Node(from), simple.Node(to), w)) + } + return g, idx +} + +// legacyComputeCommunities is a VERBATIM copy of ComputeCommunities as it stood +// immediately before #5954 S5/S6, kept as the equivalence oracle for the +// shared-CSR rewrite. It builds its own simple.WeightedUndirectedGraph from the +// directed gonum graph and walks it with gonum's interface-returning edge +// iterators, exactly as production used to. +// +// Do NOT tidy this function. Its entire value is that it is unchanged. +func legacyComputeCommunities(g *simple.WeightedDirectedGraph, idx *nodeIndex, entityNames []string, opts CommunityOptions) ([]CommunityResult, map[string]int, float64, int) { + und := legacyUndirectedProjection(g) + + const resolution = 1.0 + groups := louvainPartition(und, resolution) + + type nodeStat struct { + k float64 + cidIdx int // index into groups + degree int + } + nodeStats := make(map[int64]*nodeStat, idx.next) + for cid, gg := range groups { + for _, nid := range gg { + if _, ok := nodeStats[nid]; !ok { + nodeStats[nid] = &nodeStat{cidIdx: cid} + } else { + nodeStats[nid].cidIdx = cid + } + } + } + internalW := make([]float64, len(groups)) + var m2 float64 + wedges := und.WeightedEdges() + for wedges.Next() { + e := wedges.WeightedEdge() + w := e.Weight() + fid, tid := e.From().ID(), e.To().ID() + nf, ok := nodeStats[fid] + if !ok { + nf = &nodeStat{cidIdx: -1} + nodeStats[fid] = nf + } + nt, ok := nodeStats[tid] + if !ok { + nt = &nodeStat{cidIdx: -1} + nodeStats[tid] = nt + } + nf.k += w + nt.k += w + nf.degree++ + nt.degree++ + m2 += 2 * w // undirected: each edge contributes 2 to Σ k. + if nf.cidIdx >= 0 && nf.cidIdx == nt.cidIdx { + internalW[nf.cidIdx] += w + } + } + + K := make([]float64, len(groups)) + for cid, gg := range groups { + var k float64 + for _, nid := range gg { + if ns, ok := nodeStats[nid]; ok { + k += ns.k + } + } + K[cid] = k + } + + var overallQRaw float64 + communityQ := make([]float64, len(groups)) + if m2 > 0 { + for cid := range groups { + q := (2*internalW[cid] - resolution*K[cid]*K[cid]/m2) / m2 + communityQ[cid] = q + overallQRaw += q + } + } + overallQ := roundForDeterminism(sanitizeFloat(overallQRaw)) + + communityOf := make(map[string]int, idx.next) + for sid := range idx.toInt { + communityOf[sid] = -1 + } + results := make([]CommunityResult, 0, len(groups)) + + for cid, g := range groups { + type member struct { + id int64 + degree int + } + members := make([]member, 0, len(g)) + for _, nid := range g { + communityOf[idx.fromInt[nid]] = cid + deg := 0 + if ns, ok := nodeStats[nid]; ok { + deg = ns.degree + } + members = append(members, member{nid, deg}) + } + sort.SliceStable(members, func(i, j int) bool { + if members[i].degree != members[j].degree { + return members[i].degree > members[j].degree + } + return members[i].id < members[j].id + }) + + topN := 5 + if topN > len(members) { + topN = len(members) + } + top := make([]string, 0, topN) + for k := 0; k < topN; k++ { + eid := idx.fromInt[members[k].id] + name := nameOf(entityNames, members[k].id) + if name == "" { + name = eid + } + top = append(top, name) + } + + cQ := roundForDeterminism(sanitizeFloat(communityQ[cid])) + + results = append(results, CommunityResult{ + ID: cid, + Size: len(g), + Modularity: cQ, + TopEntities: top, + }) + } + sort.SliceStable(results, func(i, j int) bool { + if results[i].Size != results[j].Size { + return results[i].Size > results[j].Size + } + return results[i].ID < results[j].ID + }) + + minSize := opts.MinSize + if minSize < 1 { + minSize = 1 + } + var denoised int + if minSize > 1 { + kept := results[:0] + for _, r := range results { + if r.Size >= minSize { + kept = append(kept, r) + } else { + denoised++ + for eid, cid := range communityOf { + if cid == r.ID { + communityOf[eid] = -1 + } + } + } + } + results = kept + } + + return results, communityOf, overallQ, denoised +} + +// legacyUndirectedProjection is the pre-#5954-S5/S6 directed->undirected +// projection, verbatim: one gonum node walk plus one boxed weighted-edge walk, +// accumulating reciprocal pairs into a simple.WeightedUndirectedGraph. +func legacyUndirectedProjection(g *simple.WeightedDirectedGraph) *simple.WeightedUndirectedGraph { + und := simple.NewWeightedUndirectedGraph(0, 0) + nodes := g.Nodes() + for nodes.Next() { + n := nodes.Node() + if und.Node(n.ID()) == nil { + und.AddNode(simple.Node(n.ID())) + } + } + edges := g.WeightedEdges() + for edges.Next() { + e := edges.WeightedEdge() + from, to := e.From().ID(), e.To().ID() + if from == to { + continue + } + if existing := und.WeightedEdge(from, to); existing != nil { + w := existing.Weight() + e.Weight() + und.RemoveEdge(from, to) + und.SetWeightedEdge(und.NewWeightedEdge(simple.Node(from), simple.Node(to), w)) + continue + } + und.SetWeightedEdge(und.NewWeightedEdge(simple.Node(from), simple.Node(to), e.Weight())) + } + return und +} diff --git a/internal/graph/algorithms_csr_test.go b/internal/graph/algorithms_csr_test.go new file mode 100644 index 000000000..b34e8c2c6 --- /dev/null +++ b/internal/graph/algorithms_csr_test.go @@ -0,0 +1,714 @@ +package graph + +import ( + "fmt" + "math" + "math/rand/v2" + "reflect" + "sort" + "strings" + "testing" + + gonumgraph "gonum.org/v1/gonum/graph" + "gonum.org/v1/gonum/graph/simple" +) + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +// awkwardFixture builds an entity/relationship set that exercises every edge +// case BuildGraph's admission rules cover, all at once: +// +// - isolated entities (no incident relationship at all) +// - self-loop relationships +// - parallel relationships (same from/to, different weights) +// - relationships whose endpoints are not entities ("dangling") +// - relationships with a blank endpoint id +// - out-of-order relationship arrival, so the collapse cannot rely on runs +// +// Weights are deliberately irregular (callsite_count / confidence combinations +// that do not sum exactly in binary) so a re-ordered accumulation shows up as a +// bit difference rather than cancelling out. +func awkwardFixture(n int, seed uint64) ([]Entity, []Relationship) { + rng := rand.New(rand.NewPCG(seed, seed^0xa5a5a5a5)) //nolint:gosec // fixtures, not security + ents := make([]Entity, n) + for i := range ents { + ents[i] = Entity{ + ID: fmt.Sprintf("e%04d", i), + Name: fmt.Sprintf("E%d", i), + Kind: "function", + } + } + // Leave the last 10% of entities isolated on purpose. + linked := n - n/10 + if linked < 2 { + linked = n + } + + props := func() map[string]string { + return map[string]string{ + "callsite_count": fmt.Sprintf("%d", 1+rng.IntN(4)), + "confidence": fmt.Sprintf("0.%02d", 10+rng.IntN(89)), + } + } + var rels []Relationship + add := func(from, to string) { + rels = append(rels, Relationship{ + ID: fmt.Sprintf("r%d", len(rels)), + FromID: from, + ToID: to, + Kind: "CALLS", + }.WithProperties(props())) + } + for i := 0; i < linked; i++ { + for d := 0; d < 3; d++ { + add(ents[i].ID, ents[rng.IntN(linked)].ID) + } + if i%7 == 0 { + add(ents[i].ID, ents[i].ID) // self-loop + } + if i%11 == 0 { + add(ents[i].ID, "not-an-entity") + add("not-an-entity", ents[i].ID) + } + if i%13 == 0 { + add("", ents[i].ID) + add(ents[i].ID, "") + } + } + // Duplicate a chunk of the relationships (shifted) so parallel edges are + // dense, not incidental, and so duplicates are NOT adjacent in arrival + // order. + dupes := make([]Relationship, 0, len(rels)) + for i := 0; i < len(rels); i += 3 { + r := rels[i] + r.ID = "dup-" + r.ID + dupes = append(dupes, r.WithProperties(props())) + } + rels = append(rels, dupes...) + return ents, rels +} + +// gonumAdjacency reads the directed adjacency straight off the gonum graph, the +// slow-but-obviously-correct way (per-node From iterators), and returns it as +// sorted target lists plus weights. This is the oracle for directedCSR. +func gonumAdjacency(g *simple.WeightedDirectedGraph) (map[int64][]int64, map[[2]int64]float64) { + adj := make(map[int64][]int64) + w := make(map[[2]int64]float64) + nodes := gonumgraph.NodesOf(g.Nodes()) + for _, n := range nodes { + u := n.ID() + var row []int64 + to := g.From(u) + for to.Next() { + v := to.Node().ID() + row = append(row, v) + we := g.WeightedEdge(u, v) + w[[2]int64{u, v}] = we.Weight() + } + sort.Slice(row, func(i, j int) bool { return row[i] < row[j] }) + adj[u] = row + } + return adj, w +} + +// --------------------------------------------------------------------------- +// The CSR reproduces BuildGraph's structure exactly +// --------------------------------------------------------------------------- + +func TestDirectedCSRMatchesGonumAdjacencyBitForBit(t *testing.T) { + for _, tc := range []struct { + name string + n int + seed uint64 + }{ + {"tiny", 12, 1}, + {"small", 200, 2}, + {"mid", 2500, 3}, + } { + t.Run(tc.name, func(t *testing.T) { + ents, rels := awkwardFixture(tc.n, tc.seed) + g, idx := BuildGraph(ents, rels) + assertCSRMatchesLegacyBuildGraph(t, ents, rels, g, idx) + }) + } +} + +// assertCSRMatchesLegacyBuildGraph checks the CSR **and** the gonum graph that +// BuildGraph produced against legacyBuildGraph — the verbatim pre-change +// implementation — node by node, weights included. +// +// Both are checked because production now derives one from the other: g is +// materialised from the CSR, so comparing them to each other would be +// vacuous. legacyBuildGraph is the independent derivation. +func assertCSRMatchesLegacyBuildGraph(t *testing.T, ents []Entity, rels []Relationship, + g *simple.WeightedDirectedGraph, idx *nodeIndex, +) { + t.Helper() + csr := idx.csr + + if csr.n != int(idx.next) { + t.Fatalf("csr.n = %d, want %d (one row per entity)", csr.n, idx.next) + } + legacyG, legacyIdx := legacyBuildGraph(ents, rels) + if !reflect.DeepEqual(idx.toInt, legacyIdx.toInt) { + t.Fatal("node id assignment differs from the legacy implementation") + } + wantAdj, wantW := gonumAdjacency(legacyG) + + // The gonum graph BuildGraph now materialises from the CSR must itself be + // identical to the legacy one, edge for edge and weight for weight — + // ComputeCentrality's PageRank and IdentifyArticulationPoints still read it. + gotAdj, gotW := gonumAdjacency(g) + if !reflect.DeepEqual(gotAdj, wantAdj) { + t.Fatal("gonum adjacency differs from the legacy implementation") + } + for k, wv := range wantW { + if math.Float64bits(gotW[k]) != math.Float64bits(wv) { + t.Fatalf("gonum weight %v: got %v want %v", k, gotW[k], wv) + } + } + if len(gotW) != len(wantW) { + t.Fatalf("gonum edge count: got %d want %d", len(gotW), len(wantW)) + } + + if len(wantAdj) != csr.n { + t.Fatalf("legacy node count %d != csr.n %d", len(wantAdj), csr.n) + } + var edges int + for u := int32(0); u < int32(csr.n); u++ { + lo, hi := csr.row(u) + row := make([]int64, 0, hi-lo) + for p := lo; p < hi; p++ { + row = append(row, int64(csr.adj[p])) + wv := wantW[[2]int64{int64(u), int64(csr.adj[p])}] + if math.Float64bits(wv) != math.Float64bits(csr.w[p]) { + t.Fatalf("weight %d->%d: legacy=%v csr=%v (bits %x vs %x)", + u, csr.adj[p], wv, csr.w[p], + math.Float64bits(wv), math.Float64bits(csr.w[p])) + } + } + if !reflect.DeepEqual(row, wantAdj[int64(u)]) && !(len(row) == 0 && len(wantAdj[int64(u)]) == 0) { + t.Fatalf("row %d: csr=%v legacy=%v", u, row, wantAdj[int64(u)]) + } + edges += len(row) + } + if edges == 0 { + t.Fatal("fixture produced no edges — the comparison would be vacuous") + } + t.Logf("V=%d E=%d (from %d relationships)", csr.n, edges, len(rels)) +} + +// TestDirectedCSRHighFanOutRow covers the one shape neither awkwardFixture nor +// buildHeavyTailedGraph reaches: a single node with a very large out-degree, +// including parallel edges. +// +// Sheer graph size is covered elsewhere; what is specific to this shape is that +// it is the only thing that stresses (a) the `keys`/`rowW` scratch buffers, +// which are sized by the LARGEST row rather than by n, (b) the (target, arrival) +// int64 key packing at arrival indices far above zero, and (c) the hasEdge / +// weightOf binary searches over a row big enough for the search to actually +// recurse. A pathological degree distribution, not a big graph, is what changes +// what those three do. +func TestDirectedCSRHighFanOutRow(t *testing.T) { + const fanOut = 60_000 + ents := make([]Entity, fanOut+2) + for i := range ents { + ents[i] = Entity{ID: fmt.Sprintf("e%06d", i), Name: fmt.Sprintf("E%d", i), Kind: "function"} + } + hub := ents[0].ID + p := func(i int) map[string]string { + return map[string]string{ + "callsite_count": fmt.Sprintf("%d", 1+i%5), + "confidence": fmt.Sprintf("0.%02d", 11+i%80), + } + } + rels := make([]Relationship, 0, fanOut*2) + // One huge row out of the hub... + for i := 1; i <= fanOut; i++ { + rels = append(rels, Relationship{ + ID: fmt.Sprintf("r%d", i), FromID: hub, ToID: ents[i].ID, Kind: "CALLS", + }.WithProperties(p(i))) + } + // ...then a second, interleaved wave of parallel edges over every third + // target, so the collapse runs deep inside a single very long row and the + // duplicates are far apart in arrival order. + for i := 1; i <= fanOut; i += 3 { + rels = append(rels, Relationship{ + ID: fmt.Sprintf("d%d", i), FromID: hub, ToID: ents[i].ID, Kind: "CALLS", + }.WithProperties(p(i+7))) + } + // A back-edge from the far end, so the reciprocal-pair handling in the + // undirected projection also sees the huge row. + rels = append(rels, Relationship{ + ID: "back", FromID: ents[fanOut].ID, ToID: hub, Kind: "CALLS", + }.WithProperties(p(3))) + + g, idx := BuildGraph(ents, rels) + assertCSRMatchesLegacyBuildGraph(t, ents, rels, g, idx) + legacyG, _ := legacyBuildGraph(ents, rels) + + h := int32(idx.toInt[hub]) + lo, hi := idx.csr.row(h) + if hi-lo != fanOut { + t.Fatalf("hub row has %d entries, want %d after collapse", hi-lo, fanOut) + } + // Point lookups deep inside the row must agree with gonum. + for _, target := range []int{1, 2, 3, fanOut / 2, fanOut/2 + 1, fanOut - 1, fanOut} { + v := int32(idx.toInt[ents[target].ID]) + got, ok := idx.csr.weightOf(h, v) + if !ok || !idx.csr.hasEdge(h, v) { + t.Fatalf("hub->%d missing from the CSR", target) + } + // legacyG, not g: g is materialised from the CSR under test. + want := legacyG.WeightedEdge(int64(h), int64(v)).Weight() + if math.Float64bits(got) != math.Float64bits(want) { + t.Fatalf("hub->%d weight: csr=%v legacy=%v", target, got, want) + } + } + if idx.csr.hasEdge(h, h) { + t.Fatal("hub self-edge present") + } + + // The undirected projection must survive the same row. + ucsr, wantIDs := buildCSRFromDirected(idx) + want, gotIDs := buildCSRFromUndirected(legacyUndirectedProjection(g)) + if !reflect.DeepEqual(wantIDs, gotIDs) { + t.Fatal("id mapping differs") + } + if !reflect.DeepEqual(ucsr.off, want.off) || !reflect.DeepEqual(ucsr.adj, want.adj) { + t.Fatal("undirected projection differs on the high-fan-out shape") + } + assertFloatSlicesBitIdentical(t, "w", ucsr.w, want.w) + assertFloatSlicesBitIdentical(t, "k", ucsr.k, want.k) + if math.Float64bits(ucsr.m2) != math.Float64bits(want.m2) { + t.Fatalf("m2: got %v want %v", ucsr.m2, want.m2) + } + t.Logf("hub out-degree %d (from %d relationships), undirected E=%d", + hi-lo, len(rels), len(ucsr.adj)/2) +} + +// TestDirectedCSRIndexLimitIsEnforced pins that the int32 bound is a real +// check on the real path, not a comment. +// +// The failure mode it rules out is specific and silent: before enforcement, an +// oversized graph produced a CSR with n set and an all-zero offset array, which +// newBCScratch accepts happily — the result is all-zero betweenness and n +// singleton communities, PERSISTED as if they were scores. A panic is the +// correct outcome, so that is what is asserted, on both bounds, by lowering the +// limit and driving BuildGraph itself. +func TestDirectedCSRIndexLimitIsEnforced(t *testing.T) { + orig := csrIndexLimit + t.Cleanup(func() { csrIndexLimit = orig }) + + ents, rels := awkwardFixture(40, 77) + + t.Run("node count", func(t *testing.T) { + csrIndexLimit = 5 // fewer than the 40 entities + defer func() { + r := recover() + if r == nil { + t.Fatal("no panic: an oversized node count was accepted") + } + if msg, _ := r.(string); !strings.Contains(msg, "entities exceeds") { + t.Fatalf("panic message does not name the node bound: %v", r) + } + }() + BuildGraph(ents, rels) + }) + + t.Run("edge count", func(t *testing.T) { + // Above the entity count, below the admitted-relationship count, so only + // the edge bound can fire. + csrIndexLimit = 41 + if int64(len(rels)) <= csrIndexLimit { + t.Fatalf("fixture has only %d relationships — the edge bound cannot fire", len(rels)) + } + defer func() { + r := recover() + if r == nil { + t.Fatal("no panic: an oversized edge count was accepted") + } + if msg, _ := r.(string); !strings.Contains(msg, "relationships exceeds") { + t.Fatalf("panic message does not name the edge bound: %v", r) + } + }() + BuildGraph(ents, rels) + }) + + t.Run("at the limit it does not fire", func(t *testing.T) { + csrIndexLimit = orig + if _, idx := BuildGraph(ents, rels); idx.csr.n != 40 { + t.Fatalf("csr.n = %d, want 40", idx.csr.n) + } + }) +} + +// TestDirectedCSRKeepsIsolatedEntities pins behaviour 1 of BuildGraph +// INDEPENDENTLY: an entity with no incident relationship is still a node, so the +// CSR has a row for it and every later index (community id, betweenness) lines +// up with the gonum node id. +func TestDirectedCSRKeepsIsolatedEntities(t *testing.T) { + ents := []Entity{ + {ID: "a", Name: "A", Kind: "function"}, + {ID: "lonely", Name: "L", Kind: "function"}, + {ID: "b", Name: "B", Kind: "function"}, + } + rels := []Relationship{{ID: "r1", FromID: "a", ToID: "b", Kind: "CALLS"}} + _, idx := BuildGraph(ents, rels) + + if idx.csr.n != 3 { + t.Fatalf("csr.n = %d, want 3 (one row per entity, isolated included)", idx.csr.n) + } + lonely := int32(idx.toInt["lonely"]) + lo, hi := idx.csr.row(lonely) + if lo != hi { + t.Fatalf("isolated node row is non-empty: [%d,%d)", lo, hi) + } + // And the rows either side must still be addressable at the right index. + alo, ahi := idx.csr.row(int32(idx.toInt["a"])) + if ahi-alo != 1 || idx.csr.adj[alo] != int32(idx.toInt["b"]) { + t.Fatalf("row for 'a' = %v, want exactly [b]", idx.csr.adj[alo:ahi]) + } +} + +// TestDirectedCSRDropsSelfLoops pins behaviour 2 INDEPENDENTLY. +func TestDirectedCSRDropsSelfLoops(t *testing.T) { + ents := []Entity{ + {ID: "a", Name: "A", Kind: "function"}, + {ID: "b", Name: "B", Kind: "function"}, + } + rels := []Relationship{ + {ID: "r1", FromID: "a", ToID: "a", Kind: "CALLS"}, + {ID: "r2", FromID: "a", ToID: "b", Kind: "CALLS"}, + {ID: "r3", FromID: "b", ToID: "b", Kind: "CALLS"}, + } + _, idx := BuildGraph(ents, rels) + + a, b := int32(idx.toInt["a"]), int32(idx.toInt["b"]) + if idx.csr.hasEdge(a, a) || idx.csr.hasEdge(b, b) { + t.Fatal("self-loop present in the CSR") + } + if !idx.csr.hasEdge(a, b) { + t.Fatal("a->b missing") + } + if total := idx.csr.off[idx.csr.n]; total != 1 { + t.Fatalf("edge count = %d, want 1 (two self-loops dropped)", total) + } +} + +// TestDirectedCSRCollapsesParallelEdgesWithAccumulatedWeight pins behaviour 3 +// INDEPENDENTLY, and to the bit: three parallel relationships must collapse to +// one CSR entry whose weight is the same float BuildGraph's +// `w += existing.Weight()` chain produces. +func TestDirectedCSRCollapsesParallelEdgesWithAccumulatedWeight(t *testing.T) { + ents := []Entity{ + {ID: "a", Name: "A", Kind: "function"}, + {ID: "b", Name: "B", Kind: "function"}, + {ID: "c", Name: "C", Kind: "function"}, + } + p := func(count, conf string) map[string]string { + return map[string]string{"callsite_count": count, "confidence": conf} + } + // Interleaved, so the collapse cannot rely on duplicates being adjacent. + rels := []Relationship{ + Relationship{ID: "r1", FromID: "a", ToID: "b", Kind: "CALLS"}.WithProperties(p("3", "0.17")), + Relationship{ID: "r2", FromID: "a", ToID: "c", Kind: "CALLS"}.WithProperties(p("1", "0.31")), + Relationship{ID: "r3", FromID: "a", ToID: "b", Kind: "CALLS"}.WithProperties(p("7", "0.29")), + Relationship{ID: "r4", FromID: "a", ToID: "b", Kind: "CALLS"}.WithProperties(p("2", "0.83")), + } + _, idx := BuildGraph(ents, rels) + + a, b := int32(idx.toInt["a"]), int32(idx.toInt["b"]) + lo, hi := idx.csr.row(a) + if hi-lo != 2 { + t.Fatalf("row for 'a' has %d entries, want 2 (three parallel a->b collapsed)", hi-lo) + } + got, ok := idx.csr.weightOf(a, b) + if !ok { + t.Fatal("a->b missing from the CSR") + } + // Compared against legacyBuildGraph, NOT against the graph BuildGraph + // returned: production materialises that graph FROM the CSR, so comparing + // the two would be comparing a value with itself. + legacyG, _ := legacyBuildGraph(ents, rels) + want := legacyG.WeightedEdge(int64(a), int64(b)).Weight() + if math.Float64bits(got) != math.Float64bits(want) { + t.Fatalf("collapsed weight = %v, legacy = %v (bits %x vs %x)", + got, want, math.Float64bits(got), math.Float64bits(want)) + } + // Sanity: the accumulation is actually non-trivial. + single := edgeWeight(rels[0].PropsSnapshot()) + if got == single { + t.Fatalf("collapsed weight %v equals a single edge's weight — nothing accumulated", got) + } +} + +// --------------------------------------------------------------------------- +// The CSR is derived once — and consumed, not re-derived +// --------------------------------------------------------------------------- + +// TestDirectedCSRIsBuiltExactlyOncePerBuildGraph is the guard that keeps this +// change from silently unwinding. The whole saving is "derive adjacency once +// and share it"; without this, a future edit can reintroduce a second walk and +// every other test in the package still passes. +// +// It asserts both counters across a FULL pass-4 run (the path production +// actually takes), not just across BuildGraph in isolation. +func TestDirectedCSRIsBuiltExactlyOncePerBuildGraph(t *testing.T) { + t.Setenv("GRAFEL_BETWEENNESS_SAMPLE_THRESHOLD", "10") + ents, rels := awkwardFixture(600, 9) + + g, idx := BuildGraph(ents, rels) + if idx.csrBuilds != 1 { + t.Fatalf("after BuildGraph: csrBuilds = %d, want 1", idx.csrBuilds) + } + if idx.undirectedDerivations != 0 { + t.Fatalf("after BuildGraph: undirectedDerivations = %d, want 0", idx.undirectedDerivations) + } + + names := nodeNames(ents, idx) + ComputeCommunities(idx, names, DefaultCommunityOptions()) + ComputeCentrality(g, idx) + IdentifyArticulationPoints(g, idx) + + if idx.csrBuilds != 1 { + t.Errorf("after a full pass: csrBuilds = %d, want 1 — the directed adjacency is being re-derived", idx.csrBuilds) + } + if idx.undirectedDerivations != 1 { + t.Errorf("after a full pass: undirectedDerivations = %d, want 1 — the undirected projection is being re-derived", idx.undirectedDerivations) + } +} + +// TestSampledBetweennessUsesSharedCSROnTheBuildGraphPath pins that the shared +// CSR is what betweenness actually reads. A counter on construction alone would +// not catch a consumer that quietly falls back to walking the gonum graph. +func TestSampledBetweennessUsesSharedCSROnTheBuildGraphPath(t *testing.T) { + ents, rels := awkwardFixture(400, 17) + g, idx := BuildGraph(ents, rels) + + ids := make([]int64, idx.next) + for i := range ids { + ids[i] = int64(i) + } + shared := newBCScratch(g, ids, idx.csr) + if !shared.sharedCSR { + t.Fatal("newBCScratch did not take the shared-CSR path on a BuildGraph graph") + } + if &shared.adj[0] != &idx.csr.adj[0] { + t.Fatal("shared-CSR path copied the adjacency instead of aliasing it") + } + + // The gonum-derived fallback must produce byte-identical adjacency, which is + // what makes the aliasing safe. + fallback := newBCScratch(g, ids, nil) + if fallback.sharedCSR { + t.Fatal("fallback path reported sharedCSR") + } + if !reflect.DeepEqual(shared.off, fallback.off) { + t.Fatal("shared CSR offsets differ from the gonum-derived offsets") + } + if !reflect.DeepEqual(shared.adj, fallback.adj) { + t.Fatal("shared CSR adjacency differs from the gonum-derived adjacency") + } +} + +// --------------------------------------------------------------------------- +// Equivalence: Louvain / community results +// --------------------------------------------------------------------------- + +// TestUndirectedProjectionFromCSRMatchesGonumProjection compares the flat-array +// projection against the gonum route it replaced, field by field and bit by +// bit, including the derived weighted degrees and 2m. +func TestUndirectedProjectionFromCSRMatchesGonumProjection(t *testing.T) { + for _, tc := range []struct { + name string + n int + seed uint64 + }{ + {"tiny", 15, 4}, + {"small", 300, 5}, + {"mid", 3000, 6}, + } { + t.Run(tc.name, func(t *testing.T) { + ents, rels := awkwardFixture(tc.n, tc.seed) + g, idx := BuildGraph(ents, rels) + + got, gotIDs := buildCSRFromDirected(idx) + want, wantIDs := buildCSRFromUndirected(legacyUndirectedProjection(g)) + + if !reflect.DeepEqual(gotIDs, wantIDs) { + t.Fatalf("id mapping differs") + } + if got.n != want.n { + t.Fatalf("n: got %d want %d", got.n, want.n) + } + if !reflect.DeepEqual(got.off, want.off) { + t.Fatalf("offsets differ") + } + if !reflect.DeepEqual(got.adj, want.adj) { + t.Fatalf("adjacency differs") + } + assertFloatSlicesBitIdentical(t, "w", got.w, want.w) + assertFloatSlicesBitIdentical(t, "selfw", got.selfw, want.selfw) + assertFloatSlicesBitIdentical(t, "k", got.k, want.k) + if math.Float64bits(got.m2) != math.Float64bits(want.m2) { + t.Fatalf("m2: got %v want %v", got.m2, want.m2) + } + t.Logf("%s: V=%d undirected E=%d m2=%v", tc.name, got.n, len(got.adj)/2, got.m2) + }) + } +} + +func assertFloatSlicesBitIdentical(t *testing.T, label string, got, want []float64) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("%s: length %d != %d", label, len(got), len(want)) + } + for i := range got { + if math.Float64bits(got[i]) != math.Float64bits(want[i]) { + t.Fatalf("%s[%d]: got %v want %v (bits %x vs %x)", + label, i, got[i], want[i], math.Float64bits(got[i]), math.Float64bits(want[i])) + } + } +} + +// TestComputeCommunitiesMatchesLegacyGonumPath is the golden test for the +// Louvain half of the change: the entire returned tuple — community results +// (id, size, modularity, top entities), the entity->community map, the overall +// modularity, and the denoise count — must equal what the pre-change +// implementation produces, with modularity compared to FULL PRECISION. +// +// The legacy edge walk it is compared against iterates gonum's map-ordered edge +// iterator, so the legacy raw float sums are not reproducible run to run. The +// published values are quantised by roundForDeterminism, and the assertion is on +// those; the loop repeats the legacy computation so a case where the legacy side +// itself is unstable at the published precision would be caught rather than +// silently blamed on this change. +func TestComputeCommunitiesMatchesLegacyGonumPath(t *testing.T) { + for _, tc := range []struct { + name string + n int + seed uint64 + minSize int + }{ + {"tiny-nodenoise", 20, 21, 1}, + {"small-denoise", 400, 22, 5}, + {"mid-denoise", 3000, 23, 5}, + {"mid-nodenoise", 3000, 24, 1}, + } { + t.Run(tc.name, func(t *testing.T) { + ents, rels := awkwardFixture(tc.n, tc.seed) + g, idx := BuildGraph(ents, rels) + names := nodeNames(ents, idx) + opts := CommunityOptions{MinSize: tc.minSize} + + gotRes, gotOf, gotQ, gotDen := ComputeCommunities(idx, names, opts) + + for rep := 0; rep < 3; rep++ { + // legacyComputeCommunities mutates nothing shared; rebuild the + // index each rep so its own bookkeeping starts clean. + lg, lidx := BuildGraph(ents, rels) + lnames := nodeNames(ents, lidx) + wantRes, wantOf, wantQ, wantDen := legacyComputeCommunities(lg, lidx, lnames, opts) + _ = g + + if math.Float64bits(gotQ) != math.Float64bits(wantQ) { + t.Fatalf("rep %d: overall modularity got %v want %v (bits %x vs %x)", + rep, gotQ, wantQ, math.Float64bits(gotQ), math.Float64bits(wantQ)) + } + if gotDen != wantDen { + t.Fatalf("rep %d: denoised got %d want %d", rep, gotDen, wantDen) + } + if len(gotRes) != len(wantRes) { + t.Fatalf("rep %d: community count got %d want %d", rep, len(gotRes), len(wantRes)) + } + for i := range gotRes { + a, b := gotRes[i], wantRes[i] + if a.ID != b.ID || a.Size != b.Size || !reflect.DeepEqual(a.TopEntities, b.TopEntities) { + t.Fatalf("rep %d: community %d: got %+v want %+v", rep, i, a, b) + } + if math.Float64bits(a.Modularity) != math.Float64bits(b.Modularity) { + t.Fatalf("rep %d: community %d modularity got %v want %v", + rep, i, a.Modularity, b.Modularity) + } + } + if !reflect.DeepEqual(gotOf, wantOf) { + t.Fatalf("rep %d: entity->community map differs", rep) + } + } + t.Logf("%s: %d communities, Q=%v, denoised=%d", tc.name, len(gotRes), gotQ, gotDen) + }) + } +} + +// TestRunAlgorithmsEndToEndUnchanged checks the whole pass-4 output through the +// public entry point, against the same run reconstructed with the legacy +// community path — so a divergence anywhere between BuildGraph and the results +// struct shows up, not only inside the functions this change touched. +func TestRunAlgorithmsEndToEndUnchanged(t *testing.T) { + t.Setenv("GRAFEL_BETWEENNESS_SAMPLE_THRESHOLD", "10") + ents, rels := awkwardFixture(1200, 31) + + res := RunAlgorithmsWithOptions(ents, rels, DefaultCommunityOptions()) + + g, idx := BuildGraph(ents, rels) + names := nodeNames(ents, idx) + wantRes, wantOf, wantQ, wantDen := legacyComputeCommunities(g, idx, names, DefaultCommunityOptions()) + AssignCommunityNames(wantRes, ents, wantOf) + + if math.Float64bits(res.Stats.LouvainModularity) != math.Float64bits(wantQ) { + t.Fatalf("modularity got %v want %v", res.Stats.LouvainModularity, wantQ) + } + if res.Stats.DenoisedCommunities != wantDen { + t.Fatalf("denoised got %d want %d", res.Stats.DenoisedCommunities, wantDen) + } + if !reflect.DeepEqual(res.Communities, wantRes) { + t.Fatalf("community results differ") + } + if !reflect.DeepEqual(res.CommunityID, wantOf) { + t.Fatalf("entity->community map differs") + } + + // Betweenness on this path is sampled and reads the shared CSR; compare it + // against the legacy gonum-walking implementation, rounded exactly as + // ComputeCentrality rounds it. + legacyRaw := sampledBetweennessLegacy(g, betweennessSampleSize, betweennessSampleSeed) + wantBetw := make(map[string]float64, len(legacyRaw)) + for nid, v := range legacyRaw { + if rv := roundForDeterminism(sanitizeFloat(v)); rv != 0 { + wantBetw[idx.fromInt[nid]] = rv + } + } + if len(wantBetw) == 0 { + t.Fatal("fixture produced no non-zero betweenness — the comparison would be vacuous") + } + if !reflect.DeepEqual(res.Centrality, wantBetw) { + t.Fatalf("betweenness differs: got %d entries, want %d", len(res.Centrality), len(wantBetw)) + } + t.Logf("V=%d rels=%d: %d communities, Q=%v, %d scored nodes", + len(ents), len(rels), len(res.Communities), res.Stats.LouvainModularity, len(res.Centrality)) +} + +// TestDirectedCSREmptyAndEdgeless covers the degenerate shapes that the +// count-then-fill allocation could trip over. +func TestDirectedCSREmptyAndEdgeless(t *testing.T) { + _, idx := BuildGraph(nil, nil) + if idx.csr.n != 0 || len(idx.csr.off) != 1 || idx.csr.off[0] != 0 { + t.Fatalf("empty graph: csr = %+v", idx.csr) + } + if _, ids := buildCSRFromDirected(idx); len(ids) != 0 { + t.Fatalf("empty graph: undirected projection produced %d ids", len(ids)) + } + + ents := []Entity{{ID: "a", Kind: "function"}, {ID: "b", Kind: "function"}} + _, idx2 := BuildGraph(ents, nil) + if idx2.csr.n != 2 || idx2.csr.off[2] != 0 { + t.Fatalf("edgeless graph: n=%d total=%d", idx2.csr.n, idx2.csr.off[2]) + } + ucsr, _ := buildCSRFromDirected(idx2) + if ucsr.n != 2 || len(ucsr.adj) != 0 || ucsr.m2 != 0 { + t.Fatalf("edgeless projection: n=%d adj=%d m2=%v", ucsr.n, len(ucsr.adj), ucsr.m2) + } +} diff --git a/internal/graph/algorithms_sampled_test.go b/internal/graph/algorithms_sampled_test.go index 15601e0b9..0271308cb 100644 --- a/internal/graph/algorithms_sampled_test.go +++ b/internal/graph/algorithms_sampled_test.go @@ -106,7 +106,7 @@ func TestBetweennessSampleThresholdGate(t *testing.T) { // guarantees at this size (exact Betweenness is also deterministic, so we // additionally verify the dedicated sampled function matches the gated call). betw, _ := ComputeCentrality(g, idx) - direct := sampledBetweenness(g, betweennessSampleSize, betweennessSampleSeed) + direct := sampledBetweenness(g, idx.csr, betweennessSampleSize, betweennessSampleSeed) // The gated ComputeCentrality rounds for determinism; compare top-tier. gatedTop := topKByValue(betw, 10) directScaled := map[string]float64{} @@ -144,9 +144,9 @@ func TestBetweennessSampleThresholdGate(t *testing.T) { // byte-reproducible (fixed seed) across repeated calls on the same graph. func TestBetweennessSampledDeterministic(t *testing.T) { ents, rels := buildSyntheticGraph(1000, 4, 7) - g, _ := BuildGraph(ents, rels) - a := sampledBetweenness(g, 256, betweennessSampleSeed) - b := sampledBetweenness(g, 256, betweennessSampleSeed) + g, idx := BuildGraph(ents, rels) + a := sampledBetweenness(g, idx.csr, 256, betweennessSampleSeed) + b := sampledBetweenness(g, idx.csr, 256, betweennessSampleSeed) if len(a) != len(b) { t.Fatalf("non-deterministic key count: %d vs %d", len(a), len(b)) } @@ -173,7 +173,7 @@ func TestBetweennessSampledTop50Overlap(t *testing.T) { } // Sampled with the production K. - sampRaw := sampledBetweenness(g, betweennessSampleSize, betweennessSampleSeed) + sampRaw := sampledBetweenness(g, idx.csr, betweennessSampleSize, betweennessSampleSeed) samp := map[string]float64{} for nid, v := range sampRaw { samp[idx.fromInt[nid]] = v diff --git a/internal/graph/louvain.go b/internal/graph/louvain.go index f6916ac23..9bb116657 100644 --- a/internal/graph/louvain.go +++ b/internal/graph/louvain.go @@ -130,6 +130,20 @@ type csrGraph struct { // into CSR form. Nodes are indexed by ascending node id; ids[i] is the gonum // node id of index i. Adjacency lists are sorted by target index so every // downstream float summation happens in a fixed order. +// +// NO PRODUCTION CALLER since #5954 S5/S6 — production reaches Louvain through +// buildCSRFromDirected, which projects the shared directed CSR without a gonum +// graph in the middle. This survives as the adapter the Louvain QUALITY suite +// runs on (planted-partition and Barabasi-Albert fixtures are built as gonum +// undirected graphs), so every quality assertion in louvain_test.go is made +// about a graph that reached the algorithm through this function rather than +// through the production one. +// +// The single bridge that makes those assertions transfer is +// TestUndirectedProjectionFromCSRMatchesGonumProjection, which proves the two +// projections produce bit-identical csrGraphs (off/adj/w/selfw/k/m2) on the +// same input. If that test is ever weakened or deleted, the Louvain quality +// suite stops saying anything about production. func buildCSRFromUndirected(und *simple.WeightedUndirectedGraph) (*csrGraph, []int64) { var ids []int64 nodes := und.Nodes() @@ -204,6 +218,105 @@ func buildCSRFromUndirected(und *simple.WeightedUndirectedGraph) (*csrGraph, []i return g, ids } +// buildCSRFromDirected derives the UNDIRECTED projection of a directedCSR +// straight into Louvain's csrGraph, with no intermediate gonum graph. +// +// Previously ComputeCommunities materialised a simple.WeightedUndirectedGraph +// (a map-of-maps holding one boxed edge per pair) purely so that +// buildCSRFromUndirected could immediately walk it back out again. This does +// the same projection in one pass over the flat arrays. +// +// Projection semantics are identical to the gonum route: +// +// - node set is unchanged (index i is node id i, isolated nodes included); +// - a reciprocal pair u->v and v->u collapses to ONE undirected edge whose +// weight is the sum of both directions. The sum is taken with the lower +// index first; IEEE addition is commutative, so this is bit-identical to +// gonum's map-order-dependent accumulation, and unlike it, deterministic; +// - self-loops cannot occur (directedCSR drops them), so selfw stays zero. +// +// ids[i] == int64(i) because BuildGraph assigns gonum node ids 0..n-1; it is +// returned anyway so louvainPartitionFromCSR stays agnostic about that. +// +// It takes the nodeIndex rather than the directedCSR so that the +// derivation counter it bumps lands on the index: directedCSR is aliased into +// bcScratch and must have no writer after BuildGraph returns. +func buildCSRFromDirected(idx *nodeIndex) (*csrGraph, []int64) { + idx.undirectedDerivations++ + d := idx.csr + + n := d.n + g := &csrGraph{ + n: n, + off: make([]int32, n+1), + selfw: make([]float64, n), + k: make([]float64, n), + } + ids := make([]int64, n) + for i := range ids { + ids[i] = int64(i) + } + if n == 0 { + return g, ids + } + + // Pass 1 — undirected degree. Each unordered pair is counted exactly once: + // the pair is owned by whichever endpoint sees it first in ascending-u + // order, which for a reciprocal pair is the lower index. + for u := int32(0); u < int32(n); u++ { + lo, hi := d.row(u) + for p := lo; p < hi; p++ { + v := d.adj[p] + if v < u && d.hasEdge(v, u) { + continue // already counted when u' == v + } + g.off[u+1]++ + g.off[v+1]++ + } + } + for i := 0; i < n; i++ { + g.off[i+1] += g.off[i] + } + total := g.off[n] + g.adj = make([]int32, total) + g.w = make([]float64, total) + + // Pass 2 — fill, same ownership rule. + cursor := make([]int32, n) + copy(cursor, g.off[:n]) + put := func(a, b int32, w float64) { + g.adj[cursor[a]] = b + g.w[cursor[a]] = w + cursor[a]++ + g.adj[cursor[b]] = a + g.w[cursor[b]] = w + cursor[b]++ + } + for u := int32(0); u < int32(n); u++ { + lo, hi := d.row(u) + for p := lo; p < hi; p++ { + v := d.adj[p] + if v < u { + if d.hasEdge(v, u) { + continue // already emitted when u' == v + } + put(v, u, d.w[p]) + continue + } + // u < v: this endpoint owns the pair. Sum both directions. + w := d.w[p] + if rev, ok := d.weightOf(v, u); ok { + w += rev + } + put(u, v, w) + } + } + + g.sortAdjacency() + g.computeDegrees() + return g, ids +} + // sortAdjacency sorts each node's neighbour slice by target index, carrying the // parallel weight slice along. Fixed order ⇒ fixed float summation order. func (g *csrGraph) sortAdjacency() { @@ -439,8 +552,24 @@ func louvainPartition(und *simple.WeightedUndirectedGraph, resolution float64) [ // louvainMaxSweeps is a real truncation bound and silent truncation is the one // way this implementation can degrade partition quality at scale — so it has to // be observable, not inferred. +// +// Like louvainPartition and buildCSRFromUndirected, this has no production +// caller since #5954 S5/S6: it is the gonum-graph adapter the quality suite +// runs on. See buildCSRFromUndirected for why that is sound and which single +// test the soundness rests on. func louvainPartitionWithSweeps(und *simple.WeightedUndirectedGraph, resolution float64) ([][]int64, []int) { base, ids := buildCSRFromUndirected(und) + groups, sweeps := louvainPartitionFromCSR(base, ids, resolution) + return groups, sweeps +} + +// louvainPartitionFromCSR is the multi-level Louvain driver proper: it takes an +// already-built undirected CSR plus the index->gonum-id mapping and never looks +// at a gonum graph. The production path (ComputeCommunities) reaches it via +// buildCSRFromDirected, so the graph structure is walked once for the whole +// pass; louvainPartitionWithSweeps is the gonum-graph adapter kept for the +// tests that construct undirected fixtures directly. +func louvainPartitionFromCSR(base *csrGraph, ids []int64, resolution float64) ([][]int64, []int) { n := base.n if n == 0 { return nil, nil