mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-04 02:31:36 +00:00
perf(graph): reverse-adjacency + file indexes drop removeNode/removeNodesByFile from O(N)
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<nodeId, Set<relId>>` — 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<filePath, Set<nodeId>>` — 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.
This commit is contained in:
parent
41662dca38
commit
4ce1c419e3
2 changed files with 173 additions and 25 deletions
|
|
@ -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<RelationshipType, Map<string, GraphRelationship>>();
|
||||
// Reverse-adjacency index: nodeId → Set<relId> 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<string, Set<string>>();
|
||||
// File index: filePath → Set<nodeId>. Maintained on addNode /
|
||||
// removeNode so `removeNodesByFile` reaches its file's nodes
|
||||
// directly instead of scanning the whole node map.
|
||||
const nodeIdsByFile = new Map<string, Set<string>>();
|
||||
|
||||
// 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 = <K, V>(map: Map<K, Set<V>>, 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 = <K, V>(map: Map<K, Set<V>>, 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 {
|
||||
|
|
|
|||
|
|
@ -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<typeof g.addNode>[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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue