Skip to content

perf(#5954): drop the per-entity centrality pre-seed and the eager names map - #6011

Merged
cajasmota merged 1 commit into
mainfrom
worktree-agent-aa403736df84b4bd0
Jul 27, 2026
Merged

perf(#5954): drop the per-entity centrality pre-seed and the eager names map#6011
cajasmota merged 1 commit into
mainfrom
worktree-agent-aa403736df84b4bd0

Conversation

@cajasmota

Copy link
Copy Markdown
Owner

What

Removes two allocations from RunAlgorithmsWithOptions: a per-entity zero pre-seed of the betw and pr maps (~433k entries each on the reference corpus), and an eager names map[string]string held live for the whole pass.

Measured — and much smaller than estimated

~17 MB, not the ~135 MB this was scoped at. The estimate was wrong for two reasons, both found by measuring rather than reasoning:

  • pr's pre-seed saved zero bytes. network.PageRankSparse returns a score for every node and BuildGraph inserts a node per entity, so the map is fully populated one loop later regardless. Removing the pre-seed eliminates ~433k redundant map writes — a CPU win, not a memory one.
  • Betweenness is not "mostly zeros". On a realistically-connected graph it is non-zero for 91% of nodes (394,494 / 433,333, independently reproduced at 60k entities → 91.05%). Sparsifying saves ~9% of one map.

The names map was the entire prize: 21.5 MB → 6.6 MB via []string indexed by node id.

End-to-end at 433,333 entities / 2.67M rels: live heap holding the result 1004 → 1003 MB, len(Centrality) 433,333 → 394,494.

What the pre-seeded keys were load-bearing for — all three preserved

  1. IdentifyGodNodes took k = len(m)/20, and zero-scored entities filled the cut in id order when positives ran out. identifyGodNodesFrom now takes the universe from the node index: positives ranked first, then the zero tail by id. k is computed over the same universe (int(idx.next) == distinct entity count), so the cut size is unchanged.
  2. Entity.Centrality is *float64 with omitempty — nil omits the key, pointer-to-zero emits "centrality":0. Both attachment loops now gate on PageRank membership (still dense, exactly the entities in the pass) and read centrality with the zero default.
  3. groupalgo.BuildOverlay already zero-defaulted and iterates PageRank; EntityOverlay.Centrality is a non-pointer float64 with no omitempty. Overlay bytes unchanged, no OverlayAlgoVersion bump.

Verified byte-identical

Differential tests run at 536876b3a vs this commit, on fixtures built specifically for the removed pre-seed's cases — a 30-node chain (non-zero interiors), a 20-pair perfect matching (all-zero betweenness, connected), and 50 fully isolated entities:

surface base new diff
graph.json entities len(Centrality)=120, 12 god nodes len(Centrality)=28, 12 god nodes byte-identical
group overlay JSON byte-identical
god-node cut filled entirely from the zero tail (400 entities, k=20, 10 positives) 20 god nodes 20 god nodes byte-identical

Every "centrality":0 survives, including for entities absent from any relationship.

The god-node parity oracle (legacyIdentifyGodNodes) is a verbatim copy of the pre-change implementation, diffed line-by-line; it shares no code with the new path.

Test gap found in review and closed

Reverting both attachment sites to if c, ok := res.Centrality[e.ID]; ok originally passed the entire suite — precisely the edit that would silently drop "centrality":0 for ~9% of entities. The parity test mirrored the attachment loop rather than invoking it. Two new tests now call the real loops (runPass4Algorithms, attachAlgorithmResults) on a fixture containing zero-betweenness and isolated entities, and assert the serialised form contains "centrality":0 — pinning the omitempty behaviour, not just the pointer. Mutation confirmed dead at both sites.

Downgrade note (corrected)

The dashboard's graph-algo.json sidecar now serialises a sparse Centrality map. loadPersistedAlgoResults keys on the graph.fb mtime only — no binary or schema version — so this is not version-invalidated. An older binary reading a new sidecar without re-indexing leaves Centrality nil for zero-betweenness entities and omits betweenness instead of emitting 0. Cosmetic; pagerank/community/god-node/articulation unaffected; re-indexing rewrites it. The forward direction is exact.

Also

Exported IdentifyGodNodes is deleted. It ranked each map over its own key set, which was the entity universe only while the pre-seed existed — post-change an entry point taking just the two maps cannot compute the cut correctly, so leaving it exported would invite a subtly wrong call. Zero callers, zero direct tests.

Independently adversarially reviewed.

Refs #5954

The Pass-4 sweep built two entity-sized maps it did not need:

  * ComputeCentrality pre-seeded `betw` and `pr` with a 0 for EVERY entity
    so callers could rely on the key being present. `pr` is repopulated in
    full one loop later (PageRankSparse returns a score per node), so its
    pre-seed was pure work; `betw` is genuinely sparse — gonum reports
    non-zero betweenness only.
  * RunAlgorithmsWithOptions built a map[string]string over every entity to
    answer at most 5 name lookups per community.

Both are gone. The betweenness map now holds non-zero scores only (zeros
are dropped, so "absent" is the single representation of zero), and
community naming reads a []string indexed by the gonum node id BuildGraph
already assigned.

Output is unchanged. What the pre-seeded keys were actually load-bearing
for, and how each is preserved:

  * IdentifyGodNodes took its 5% cut over len(map) and let zero-scored
    entities fill the cut in id order. identifyGodNodesFrom now takes the
    universe from the node index instead, ranking positives first and then
    the zero tail by id — the same selection, proven against the verbatim
    pre-#5954 implementation in TestGodNodesParityWithZeroPreSeed
    (including a fixture whose cut is filled ENTIRELY from the zero tail).
    The exported IdentifyGodNodes is DELETED: it ranked each map over its
    own keys, which was the entity universe only while the pre-seed existed,
    so an entry point taking just the two maps can no longer compute the cut.
    It had no callers.
  * The entity-attachment loops (cmd/grafel/index.go, dashboard's
    applyAlgorithmResults) tested Centrality for key presence to decide
    whether to set Entity.Centrality, which is `*float64` with omitempty —
    an absent key would have dropped `"centrality": 0` from graph.json.
    They now key off PageRank membership, which still covers every entity
    in the pass, and read centrality with the zero default. Both sites are
    guarded by a zero-betweenness fixture
    (TestPass4Attachment_ZeroBetweennessKeepsExplicitCentrality,
    TestApplyAlgorithmResults_ZeroBetweennessKeepsExplicitCentrality); the
    pre-existing tests only covered entities with NON-zero betweenness, so
    reverting either site used to pass the whole suite.
  * groupalgo.BuildOverlay already read centrality with a zero default, so
    <group>-algo.json still records an explicit 0 per entity. Its inverse,
    overlayToResults, drops zero centralities so a reconstituted result
    stays DeepEqual to a full recompute (the #5309 incremental parity test).

Measured on the membench fixture at 433,333 entities / 2.67M rels:

  before: total-alloc 39936 MB / 336,858,194 allocs, len(Centrality)=433,333
  after:  total-alloc 39919 MB / 336,856,980 allocs, len(Centrality)=394,494

so ~17 MB and ~1.2k allocs — NOT the ~135 MB the epic estimated. Isolated
at the same scale: the name map costs 21.5 MB against 6.6 MB for the slice,
and betweenness is non-zero for ~91% of nodes on a connected graph, so the
sparse map only saves ~1.1 MB of the 14.4 MB it used to cost. The `pr`
pre-seed was worth 0 bytes — the map ends up the same size either way.

One artifact does change shape without changing meaning: the dashboard's
graph-algo.json sidecar JSON-encodes AlgorithmResults, so its Centrality
object now omits zero entries. Forward is exact — a dense sidecar written by
an older binary reads correctly under the new PageRank-keyed attachment. The
downgrade direction is NOT protected: loadPersistedAlgoResults keys the
sidecar on the graph.fb mtime alone (no binary or schema version), so an
older binary reading a new sparse sidecar WITHOUT re-indexing would leave
Centrality nil for zero-betweenness entities and handlers_graph.go would
omit `betweenness` instead of reporting 0. Cosmetic (a JS reader sees
undefined rather than 0); pagerank, community, god-node and articulation are
unaffected; re-indexing rewrites the sidecar.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant