From 4ce1c419e353df2617e066c3ebeffff4760d2440 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 21 Apr 2026 13:13:21 +0100 Subject: [PATCH] perf(graph): reverse-adjacency + file indexes drop removeNode/removeNodesByFile from O(N) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #980 in-line review flagged that `removeNode` iterated the full relationshipMap to find edges touching a node (O(E)), and `removeNodesByFile` called removeNode for every matching node after a full nodeMap scan (O(N × E)). Pre-existing, but worth fixing properly since the writeRel/deleteRel helpers we just added make the index-maintenance story coherent. Two new indexes maintained on every mutation path: - `edgeIdsByNode: Map>` — reverse adjacency. Every edge records both endpoints, so removeNode iterates edgeIdsByNode.get(id) instead of every relationship. Self-edges skip the duplicate-endpoint write to keep the Set dedup explicit. - `nodeIdsByFile: Map>` — file index. removeNodesByFile reaches its file's nodes directly. Complexity: - removeNode: O(edges-touching-node), was O(total-edges). - removeNodesByFile: O(file-nodes × avg-edges-per-node + scan of the file bucket), was O(total-nodes + file-nodes × total-edges). Index maintenance is centralized in writeRel/deleteRel + new addToBucket/removeFromBucket helpers. Empty buckets are pruned to keep the indexes compact. Existing dual-invariant (relationshipMap ↔ relationshipsByType) preserved. Nodes without a `filePath` property (e.g. Community/Cluster nodes) are intentionally NOT indexed in nodeIdsByFile — they can't belong to any file, so removeNodesByFile correctly leaves them alone. Coverage: 7 new unit tests (33/33 total, was 26). Added cases: - removes only edges touching the removed node - handles self-edges - removes orphan node with no edges - removeNodesByFile removes only matching nodes - returns 0 when no match - also removes edges whose endpoints lived on the removed file - does not index nodes without a filePath property Verification: 204/204 test/integration/resolvers/python.test.ts both REGISTRY_PRIMARY_PYTHON=0 and =1. 4235/4235 unit tests. tsc clean. --- gitnexus/src/core/graph/graph.ts | 104 +++++++++++++++++++++++-------- gitnexus/test/unit/graph.test.ts | 94 ++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 25 deletions(-) diff --git a/gitnexus/src/core/graph/graph.ts b/gitnexus/src/core/graph/graph.ts index 1dbe701b9..c906e1b10 100644 --- a/gitnexus/src/core/graph/graph.ts +++ b/gitnexus/src/core/graph/graph.ts @@ -16,28 +16,70 @@ export const createKnowledgeGraph = (): KnowledgeGraph => { // and per-edge removal is O(1). See plan // docs/plans/2026-04-20-002-perf-parse-heritage-mro-plan.md (Unit 1). const relationshipsByType = new Map>(); + // Reverse-adjacency index: nodeId → Set of every edge where + // this node appears as source OR target. Maintained on writeRel / + // deleteRel so `removeNode` can delete a node's edges in + // O(edges-touching-node) instead of O(total-edges). + const edgeIdsByNode = new Map>(); + // File index: filePath → Set. Maintained on addNode / + // removeNode so `removeNodesByFile` reaches its file's nodes + // directly instead of scanning the whole node map. + const nodeIdsByFile = new Map>(); + + // Private helpers that encode the dual-index invariants in one + // place. All mutation paths go through these — adding a new + // mutation method only needs to call the helper, not remember to + // touch every index. + const addToBucket = (map: Map>, key: K, value: V): void => { + let bucket = map.get(key); + if (bucket === undefined) { + bucket = new Set(); + map.set(key, bucket); + } + bucket.add(value); + }; + const removeFromBucket = (map: Map>, key: K, value: V): void => { + const bucket = map.get(key); + if (bucket === undefined) return; + bucket.delete(value); + if (bucket.size === 0) map.delete(key); + }; - // Private helpers that encode the dual-index invariant in one place. - // All mutation paths (addRelationship, removeRelationship, removeNode) - // go through these — adding a new mutation method only needs to call - // writeRel / deleteRel, not remember to touch both maps. const writeRel = (rel: GraphRelationship): void => { relationshipMap.set(rel.id, rel); - let bucket = relationshipsByType.get(rel.type); - if (bucket === undefined) { - bucket = new Map(); - relationshipsByType.set(rel.type, bucket); + let typeBucket = relationshipsByType.get(rel.type); + if (typeBucket === undefined) { + typeBucket = new Map(); + relationshipsByType.set(rel.type, typeBucket); + } + typeBucket.set(rel.id, rel); + addToBucket(edgeIdsByNode, rel.sourceId, rel.id); + // Guard against a self-edge writing the same rel.id into the + // same Set twice — Set dedup handles it, but we skip explicitly + // for clarity. + if (rel.targetId !== rel.sourceId) { + addToBucket(edgeIdsByNode, rel.targetId, rel.id); } - bucket.set(rel.id, rel); }; const deleteRel = (rel: GraphRelationship): void => { relationshipMap.delete(rel.id); - relationshipsByType.get(rel.type)?.delete(rel.id); + const typeBucket = relationshipsByType.get(rel.type); + if (typeBucket !== undefined) { + typeBucket.delete(rel.id); + if (typeBucket.size === 0) relationshipsByType.delete(rel.type); + } + removeFromBucket(edgeIdsByNode, rel.sourceId, rel.id); + if (rel.targetId !== rel.sourceId) { + removeFromBucket(edgeIdsByNode, rel.targetId, rel.id); + } }; const addNode = (node: GraphNode) => { - if (!nodeMap.has(node.id)) { - nodeMap.set(node.id, node); + if (nodeMap.has(node.id)) return; + nodeMap.set(node.id, node); + const filePath = node.properties?.filePath; + if (typeof filePath === 'string' && filePath.length > 0) { + addToBucket(nodeIdsByFile, filePath, node.id); } }; @@ -47,17 +89,29 @@ export const createKnowledgeGraph = (): KnowledgeGraph => { }; /** - * Remove a single node and all relationships involving it + * Remove a single node and all relationships involving it. + * O(edges-touching-node) via the reverse-adjacency index — no full + * relationshipMap scan. */ const removeNode = (nodeId: string): boolean => { - if (!nodeMap.has(nodeId)) return false; + const node = nodeMap.get(nodeId); + if (node === undefined) return false; nodeMap.delete(nodeId); + const filePath = node.properties?.filePath; + if (typeof filePath === 'string' && filePath.length > 0) { + removeFromBucket(nodeIdsByFile, filePath, nodeId); + } - for (const rel of relationshipMap.values()) { - if (rel.sourceId === nodeId || rel.targetId === nodeId) { - deleteRel(rel); + const touchingEdgeIds = edgeIdsByNode.get(nodeId); + if (touchingEdgeIds !== undefined) { + // Snapshot the ids before iterating — deleteRel mutates the same + // Set via removeFromBucket, which would break mid-loop iteration. + for (const relId of [...touchingEdgeIds]) { + const rel = relationshipMap.get(relId); + if (rel !== undefined) deleteRel(rel); } + edgeIdsByNode.delete(nodeId); } return true; }; @@ -75,16 +129,16 @@ export const createKnowledgeGraph = (): KnowledgeGraph => { /** * Remove all nodes (and their relationships) belonging to a file. + * O(file-nodes × avg-edges-per-node) via the file index — no full + * node-map scan. */ const removeNodesByFile = (filePath: string): number => { - let removed = 0; - for (const [nodeId, node] of nodeMap) { - if (node.properties?.filePath === filePath) { - removeNode(nodeId); - removed++; - } - } - return removed; + const nodeIds = nodeIdsByFile.get(filePath); + if (nodeIds === undefined) return 0; + // Snapshot before iterating — removeNode mutates nodeIdsByFile. + const snapshot = [...nodeIds]; + for (const nodeId of snapshot) removeNode(nodeId); + return snapshot.length; }; return { diff --git a/gitnexus/test/unit/graph.test.ts b/gitnexus/test/unit/graph.test.ts index 7244ec572..47be5125d 100644 --- a/gitnexus/test/unit/graph.test.ts +++ b/gitnexus/test/unit/graph.test.ts @@ -326,4 +326,98 @@ describe('createKnowledgeGraph', () => { expect(g.relationshipCount).toBe(1); }); }); + + // ─── Reverse-adjacency + file indexes ───────────────────────────── + // Pin the behavior of the nodeIdsByFile + edgeIdsByNode indexes + // that back removeNode / removeNodesByFile. These replace the prior + // O(N) full-map scans with O(edges-touching-node) and + // O(file-nodes × avg-edges-per-node) respectively. + + describe('removeNode reverse-adjacency', () => { + it('removes only edges touching the removed node', () => { + const g = createKnowledgeGraph(); + g.addNode(makeNode('fn:a', 'a', 'src/a.ts')); + g.addNode(makeNode('fn:b', 'b', 'src/a.ts')); + g.addNode(makeNode('fn:c', 'c', 'src/c.ts')); + g.addRelationship(makeRel('fn:a', 'fn:b')); + g.addRelationship(makeRel('fn:b', 'fn:c')); + g.addRelationship(makeRel('fn:a', 'fn:c')); + + g.removeNode('fn:b'); + + // Two edges touched fn:b (a→b and b→c); only a→c survives. + expect(g.relationshipCount).toBe(1); + const survivors = [...g.iterRelationships()]; + expect(survivors[0].sourceId).toBe('fn:a'); + expect(survivors[0].targetId).toBe('fn:c'); + }); + + it('handles self-edges without double-counting or crashing', () => { + const g = createKnowledgeGraph(); + g.addNode(makeNode('fn:a', 'a', 'src/a.ts')); + g.addRelationship(makeRel('fn:a', 'fn:a')); + expect(g.relationshipCount).toBe(1); + + g.removeNode('fn:a'); + expect(g.relationshipCount).toBe(0); + expect(g.nodeCount).toBe(0); + }); + + it('removes orphan node with no edges cleanly', () => { + const g = createKnowledgeGraph(); + g.addNode(makeNode('fn:a', 'a', 'src/a.ts')); + expect(g.removeNode('fn:a')).toBe(true); + expect(g.nodeCount).toBe(0); + }); + }); + + describe('removeNodesByFile via file index', () => { + it('removes only nodes matching the file path', () => { + const g = createKnowledgeGraph(); + g.addNode(makeNode('fn:a', 'a', 'src/a.ts')); + g.addNode(makeNode('fn:b', 'b', 'src/a.ts')); + g.addNode(makeNode('fn:c', 'c', 'src/other.ts')); + + const removed = g.removeNodesByFile('src/a.ts'); + expect(removed).toBe(2); + expect(g.nodeCount).toBe(1); + expect(g.getNode('fn:c')).toBeDefined(); + }); + + it('returns 0 when no node matches the file path', () => { + const g = createKnowledgeGraph(); + g.addNode(makeNode('fn:a', 'a', 'src/a.ts')); + expect(g.removeNodesByFile('src/missing.ts')).toBe(0); + expect(g.nodeCount).toBe(1); + }); + + it('also removes edges whose endpoints lived on the removed file', () => { + const g = createKnowledgeGraph(); + g.addNode(makeNode('fn:a', 'a', 'src/a.ts')); + g.addNode(makeNode('fn:b', 'b', 'src/b.ts')); + g.addRelationship(makeRel('fn:a', 'fn:b')); + expect(g.relationshipCount).toBe(1); + + g.removeNodesByFile('src/a.ts'); + // Removing fn:a also removed the a→b edge; fn:b survives. + expect(g.nodeCount).toBe(1); + expect(g.relationshipCount).toBe(0); + }); + + it('does not index nodes without a filePath property', () => { + const g = createKnowledgeGraph(); + // Cluster/Community nodes and similar have no filePath. + const node: Parameters[0] = { + id: 'cluster:x', + label: 'Community', + properties: { name: 'x' }, + }; + g.addNode(node); + g.addNode(makeNode('fn:a', 'a', 'src/a.ts')); + + expect(g.removeNodesByFile('src/a.ts')).toBe(1); + expect(g.nodeCount).toBe(1); + expect(g.getNode('cluster:x')).toBeDefined(); + }); + }); });