perf(#5954): stop re-inflating compacted entity properties into a map in the link pass - #6009
Merged
Merged
Conversation
…flating maps (slice 2) #5976 compacted RelationshipRecord/EntityRecord properties from map[string]string to the sorted-slice types.Props. loadAllGraphs then undid that for every entity in the group union: entityNode.Properties was a map[string]string built via PropsSnapshot, one Go map header + bucket per entity, ~427k entities on the reference corpus. entityNode.Properties is now types.Props, fed by a new graph.Entity.PropsClone() that hands over the sorted slice directly. All ~40 read/write sites across the 14 property-touching passes go through the Props accessors (Get/Lookup/Set); no .Snapshot()/PropsFromMap bounce was added at any of them. MEASURED SAVING — allocation, not resident heap Every committed test here measures BYTES ALLOCATED (MemStats.TotalAlloc). No committed test pins RSS, so no RSS figure is claimed. On a realistic property-count distribution (25% of entities with 0 props, 50% with 2, 15% with 5, 10% with 12 incl. long keys/values), pinned by TestEntityNodeProjection_RealisticDistributionSaving: Props 94.4 B/entity vs map 284.8 B/entity => 190.4 B/entity saved => ~81 MB at 427k entities An earlier draft of this commit quoted ~119 MB. That came from a uniform 2-property fixture, which is near the map's worst case and the slice's best; it overstates the corpus saving by roughly a third. The 2-property fixture is retained as the regression detector (it maximises signal for a ceiling assertion) but is explicitly no longer the headline number. Steady state is lower again: once the stamping passes add ~4 more keys per entity the two representations converge further (hand-measured ~76 MB). BYTE-IDENTICAL OUTPUT, AND THE LIMITS OF THAT GATE entity_props_diff_5954_test.go runs the full pass pipeline and compares all 9 emitted group files byte for byte against goldens captured from the pre-change map-backed implementation. A companion test proves those bytes are deterministic run-to-run. The gate is narrower than it looks and the file now says so. Per the committed g5954-link-pass-stats.json, only import / http / reachability / label / module_cycles emit anything; constant_propagation, effect_propagation, taint_flow, openapi-spec, same_as, payload_drift, pure_functions, def_use, complexity and data_flow all report links_added=0, candidates=0. Coverage is ~3 of the 14 converted passes. Six converted Set calls at four locations are covered by no test in the package, before or after this change: def_use_pass.go:178-179, module_cycle_pass.go:145, pure_function_pass.go:91, dataflow_pass.go:552-553. All are unbranched m[k]=v -> p.Set(k,v) rewrites. Recorded as a pre-existing gap, not waved off. Mutation results worth recording: giving types.Props.Set duplicate-insert semantics at the correct sorted position (breaking last-write-wins while preserving key order) was caught by NOTHING in internal/links — both golden tests passed and the whole package stayed green. Only internal/types.TestPropsMatchesMapOracle failed. The cruder append-at-end shape, which also breaks sort order, is caught loudly by ~11 tests here. So this package's sensitivity comes from key ORDER being observable in output, not from key SEMANTICS being asserted. GATE STRENGTH TestEntityNodeProjection_MapReintroductionWouldFail now bounds the 150-byte ceiling against the CHEAPEST re-inflation shape (types.PropsFromMap(e.PropsSnapshot()), measured 168 B/entity) rather than against a bare map (336 B/entity). Previously the combined mutation "raise the ceiling to 200 AND reintroduce the map" passed both alloc tests; it now fails. TestLinkPass_NoPropertyMapBounceInProductionCode is load-bearing, not belt-and-braces: it is the only test that catches a re-inflation added anywhere other than newEntityNode. Its guarantee is stated narrowly — it is a textual check for .Snapshot()/.PropsSnapshot() in non-test files of this package, so a hand-rolled shadow map built via Range, or any conversion in cmd/, slips past it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
entityNode.Propertiesin the link pass carriedmap[string]string, rebuilt from the already-compactedtypes.Props. It now carriestypes.Propsdirectly.Why
#5976 compacted
RelationshipRecord.Propertiesfrom a map to a[]propKV-backedtypes.Props, banking ~436 MB. The link pass then re-inflated it on load: one map header plus buckets per entity, across ~427k entities. This is self-inflicted waste rather than new engineering — it gives back part of a win we had already paid for.Measured, not estimated
Sets)BenchmarkEntityNodeProjection: 336 B/op, 2 allocs → 64 B/op, 1 alloc.An earlier revision of this PR claimed ~119 MB. That came from a fixture with a uniform 2 properties per entity — near the map's worst case and the slice's best — and overstated the win by about a third. The uniform fixture is retained, but as the regression detector (a tight ceiling is exactly what you want there), not as the corpus estimate.
The committed tests measure allocation via
TotalAlloc, not RSS. No committed test pins resident heap.Translation
entityNode.Propertiesis populated by a newgraph.Entity.PropsClone()— one exact-sized slice copy, no map. All read sites useGet/Lookup; all write sites useSet. Zero.Snapshot()/PropsFromMapbounces were added in non-test code.Converted:
openapi_pass,http_pass,constant_propagation,effect_propagation(signature changedmap[string]string→*types.Props),sameas_pass,complexity_pass,dataflow_pass,def_use_pass,pure_function_pass,reachability,taint_flow,module_cycle_pass.Link.Propertiesdeliberately stays a map: it is built fresh by the matchers, never re-inflated from a compacted form, and there are thousands of links rather than hundreds of thousands of entities.Semantic equivalence
Independently verified by review, since map and slice diverge in four ways that matter:
types.Props{}is non-nil, andPropsClone/PropsSnapshotboth return nil for an empty set, so every== nilguard translates exactly.Setbinary-searches, then overwrites in place or inserts at the sorted position.Properties, every read is keyed.range …Entitiessites that mutate use&g.Entities[ei]. Inserting through a value copy would be visible with a map and lost with a slice; no such site exists.One dependency this change newly extends to the link pass:
Props.Getbinary-searches, so an unsorted backing slice silently returns"". Verified the invariant holds at both producers —fbwriter/writer.go:284-288sorts before emission,load.go:541-556relies on exactly that, and the JSON path sorts viapropsFromMap.Entity.Prop()already binary-searched, so the invariant was load-bearing before; the blast radius is wider now.Byte-identical output
9 golden files in
testdata/linkout_5954/, generated from the pre-change map-backed implementation. The reviewer did not take that on trust: they built a detached worktree at the base commit, copied in only the diff test, regenerated, anddiff -r'd against the committed goldens — identical. That also proves pre-change output was already deterministic, so there was no map-iteration-order leak for this change to alter.Coverage is narrower than "9 goldens" suggests, and this is recorded rather than implied: only import, http, reachability, label and module_cycles emit anything; ten passes report
links_added=0. The gate covers ~3 of the 14 converted passes. SixSetcalls at four locations (def_use_pass.go:178-179,module_cycle_pass.go:145,pure_function_pass.go:91,dataflow_pass.go:552-553) are untested by anything in the package — all straightm[k]=v→p.Set(k,v)with no branching, and equally untested before.Gate strength
TestEntityNodeProjection_MapReintroductionWouldFailbounds the ceiling against the cheapest re-inflation shape (PropsFromMap(PropsSnapshot()), 168 B/entity), not a bare map (336). Review found that a ceiling asserted only against 336 would admit a 200-byte limit that lets the 168-byte bounce through; that hole is now closed.TestLinkPass_NoPropertyMapBounceInProductionCodeis documented as load-bearing — it is the only test catching a re-inflation added anywhere other thannewEntityNode— with its limits stated: a textual match on.Snapshot()/.PropsSnapshot()in non-test files of this package. A shadow map built viaRange, or anything undercmd/, slips past.Mutation results
Killed: map bounce in
newEntityNode(both the byte gate and the static guard); ceiling loosened to 400;.Snapshot()added insidereachability.go; duplicate-keySet.Recorded honestly: the duplicate-key kill is shape-dependent. Appending at the end breaks sort order and fails ~11 tests here. Inserting a duplicate at the correct sorted position is caught by nothing in
internal/links— both golden tests pass — and onlyinternal/types.TestPropsMatchesMapOraclefails. So this package's sensitivity comes from key order being observable in serialised output, not from key semantics being asserted anywhere.Follow-up found by review
types.EntityRecord.Propertiesis stillmap[string]string, socmd/grafel/index.go:1197doesProperties: pe.PropsSnapshot()on a per-entity loop — the same re-inflation, on the write path, outside this package's static guard. That is the next propKV slice, tracked separately.Independently adversarially reviewed.
Refs #5954