From 60aa7efa9a337960da7c29f5fd3ee0ba715e0c66 Mon Sep 17 00:00:00 2001 From: Sonu Kapoor Date: Sun, 14 Jun 2026 09:55:31 -0400 Subject: [PATCH] perf(npm-lock-graph): use Set accumulators for edge lists to reduce graph construction from O(E*V) to O(E) Refs #645 --- src/parsers/npm-lock-graph.ts | 49 ++++++++++++++++------------ tests/parsers/npm-lock-graph.test.ts | 49 ++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 20 deletions(-) diff --git a/src/parsers/npm-lock-graph.ts b/src/parsers/npm-lock-graph.ts index 5f4eab4e..25a839d3 100644 --- a/src/parsers/npm-lock-graph.ts +++ b/src/parsers/npm-lock-graph.ts @@ -22,12 +22,12 @@ export function loadNpmLockGraph( const packages = raw?.packages; const rawPackages = isRecord(packages) ? (packages as Record) : {}; const nodesById = new Map(); - const nodeIdsByPackageKey = new Map(); + const nodeIdsByPackageKey = new Map>(); const nodeIdByPackagePath = new Map(); const dependencyRangesByNodeId = new Map>(); const resolutionBasePathsByNodeId = new Map(); - const parentNodeIdsByChildNodeId = new Map(); - const childNodeIdsByParentNodeId = new Map(); + const parentNodeIdsByChildNodeId = new Map>(); + const childNodeIdsByParentNodeId = new Map>(); const rangeByParentNodeId = new Map>(); const pathSetByNodeId = new Map>(); @@ -35,9 +35,9 @@ export function loadNpmLockGraph( return createGraph({ entryPackages: [], nodesById, - nodeIdsByPackageKey, - parentNodeIdsByChildNodeId, - childNodeIdsByParentNodeId, + nodeIdsByPackageKey: setsToArrays(nodeIdsByPackageKey), + parentNodeIdsByChildNodeId: setsToArrays(parentNodeIdsByChildNodeId), + childNodeIdsByParentNodeId: setsToArrays(childNodeIdsByParentNodeId), rangeByParentNodeId, pathSetByNodeId, }); @@ -65,7 +65,9 @@ export function loadNpmLockGraph( dev: !!meta?.dev, }); nodeIdByPackagePath.set(packagePath, id); - nodeIdsByPackageKey.set(packageKey, [...(nodeIdsByPackageKey.get(packageKey) ?? []), id]); + const keySet = nodeIdsByPackageKey.get(packageKey) ?? new Set(); + keySet.add(id); + nodeIdsByPackageKey.set(packageKey, keySet); dependencyRangesByNodeId.set(id, collectDependencyRanges(resolveDependencySource(meta, rawPackages))); resolutionBasePathsByNodeId.set(id, resolveBasePaths(packagePath, meta)); } @@ -130,9 +132,9 @@ export function loadNpmLockGraph( return createGraph({ entryPackages, nodesById, - nodeIdsByPackageKey, - parentNodeIdsByChildNodeId, - childNodeIdsByParentNodeId, + nodeIdsByPackageKey: setsToArrays(nodeIdsByPackageKey), + parentNodeIdsByChildNodeId: setsToArrays(parentNodeIdsByChildNodeId), + childNodeIdsByParentNodeId: setsToArrays(childNodeIdsByParentNodeId), rangeByParentNodeId, pathSetByNodeId, }); @@ -180,8 +182,8 @@ function resolveEdgesForParent( dependencyRanges: Record, resolutionBasePaths: string[], nodeIdByPackagePath: Map, - childNodeIdsByParentNodeId: Map, - parentNodeIdsByChildNodeId: Map, + childNodeIdsByParentNodeId: Map>, + parentNodeIdsByChildNodeId: Map>, rangeByParentNodeId: Map>, ): string[] { const resolvedChildNodeIds: string[] = []; @@ -198,14 +200,13 @@ function resolveEdgesForParent( const parentNodeId = parentPackagePath ? nodeIdByPackagePath.get(parentPackagePath) : null; if (!parentNodeId) continue; - childNodeIdsByParentNodeId.set( - parentNodeId, - unique([...(childNodeIdsByParentNodeId.get(parentNodeId) ?? []), childNodeId]), - ); - parentNodeIdsByChildNodeId.set( - childNodeId, - unique([...(parentNodeIdsByChildNodeId.get(childNodeId) ?? []), parentNodeId]), - ); + const childSet = childNodeIdsByParentNodeId.get(parentNodeId) ?? new Set(); + childSet.add(childNodeId); + childNodeIdsByParentNodeId.set(parentNodeId, childSet); + + const parentSet = parentNodeIdsByChildNodeId.get(childNodeId) ?? new Set(); + parentSet.add(parentNodeId); + parentNodeIdsByChildNodeId.set(childNodeId, parentSet); const rangesForParent = rangeByParentNodeId.get(parentNodeId) ?? new Map(); rangesForParent.set(dependencyName, range); @@ -301,6 +302,14 @@ function rememberPath(pathSetByNodeId: Map>, nodeId: string, pathSetByNodeId.set(nodeId, existing); } +function setsToArrays(map: Map>): Map { + const result = new Map(); + for (const [key, set] of map) { + result.set(key, [...set]); + } + return result; +} + function isRecord(value: unknown): value is Record { return !!value && typeof value === "object"; } diff --git a/tests/parsers/npm-lock-graph.test.ts b/tests/parsers/npm-lock-graph.test.ts index 347531c4..fc705d84 100644 --- a/tests/parsers/npm-lock-graph.test.ts +++ b/tests/parsers/npm-lock-graph.test.ts @@ -337,6 +337,55 @@ describe("npm lock graph extraction", () => { } }); + it("produces no duplicate entries in childrenFor/parentsFor when the same edge appears multiple times in the lockfile", () => { + // Regression test for the Set accumulator refactor (PR #652). + // The old unique([...spread]) pattern deduplicated on every insert. + // The new Set.add() approach must produce the same deduplicated result + // even if the same child-parent pair is encountered more than once + // during graph construction. + const projectDir = createTempProjectDir(); + const lockPath = path.join(projectDir, "package-lock.json"); + + fs.writeFileSync( + lockPath, + JSON.stringify({ + name: "fixture", + lockfileVersion: 3, + packages: { + "": { + name: "fixture", + version: "1.0.0", + dependencies: { parent: "^1.0.0" }, + }, + "node_modules/parent": { + version: "1.0.0", + // child appears in both dependencies and optionalDependencies - + // two declarations of the same edge that should collapse to one. + dependencies: { child: "^1.0.0" }, + optionalDependencies: { child: "^1.0.0" }, + }, + "node_modules/child": { version: "1.0.0" }, + }, + }), + "utf8", + ); + + try { + const graph = loadNpmLockGraph(lockPath, { includePaths: true }); + const parentId = graph.nodeIdsFor("parent", "1.0.0")[0]!; + const childId = graph.nodeIdsFor("child", "1.0.0")[0]!; + + const children = graph.childrenFor(parentId); + const parents = graph.parentsFor(childId); + + // Each should appear exactly once despite two declarations of the same edge + expect(children.filter(id => id === childId)).toHaveLength(1); + expect(parents.filter(id => id === parentId)).toHaveLength(1); + } finally { + removeDir(projectDir); + } + }); + it("bounds path depth and terminates on graphs with dependency cycles", () => { // pkg-a → pkg-b → pkg-c → pkg-a (cycle) const projectDir = createTempProjectDir();