From 51aac62e9a3602b6566cd4f2e7ce3dfacf5190b3 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 27 May 2026 20:21:12 -0400 Subject: [PATCH] perf(#307): use head-index queue for phase dependency BFS (O(V^2) -> O(V+E)) (#383) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Pass-2 topological level assignment in cmdPhasePlanIndex dequeued its Kahn's-algorithm queue with Array.shift(), which is O(n) per call in V8, so the BFS was O(V^2) and slowed superlinearly on deep queues (wide fan-in plan graphs). Extract the traversal into a pure, exported computeDependencyLevels (rawPlans, planMap, canonicalToId) and dequeue via a head index (queue[head++]) -> O(V+E). Behavior is identical: same FIFO order, same longest-path levels, same visited-count cycle detection. A complexity-contract comment above the loop documents why shift() must not be reintroduced. Adds tests/phase-dependency-levels.test.cjs with deterministic behavior and edge-case coverage (linear chain, diamond longest-path, independent set, cycle, canonical-prefix resolution, empty, self-loop, duplicate edge, external dep). A timing-based complexity guard was intentionally omitted: the O(V+E) Map-build constant dilutes the O(V^2) signal until impractical N (~1e6), so an empirical guard is inherently flaky on contended CI — the contract is enforced by the inline comment and correctness tests instead. Fixes #307 Co-authored-by: Claude Opus 4.7 (1M context) --- .changeset/jolly-moles-climb.md | 5 + get-shit-done/bin/lib/phase.cjs | 105 +++++++++-------- tests/phase-dependency-levels.test.cjs | 154 +++++++++++++++++++++++++ 3 files changed, 219 insertions(+), 45 deletions(-) create mode 100644 .changeset/jolly-moles-climb.md create mode 100644 tests/phase-dependency-levels.test.cjs diff --git a/.changeset/jolly-moles-climb.md b/.changeset/jolly-moles-climb.md new file mode 100644 index 000000000..1928c5cbf --- /dev/null +++ b/.changeset/jolly-moles-climb.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 383 +--- +Phase dependency level assignment (gsd-tools phase-plan-index) now dequeues its Kahns-algorithm BFS via a head index instead of Array.shift(), fixing O(V^2) behavior that slowed superlinearly on wide fan-in plan graphs (Array.shift() is O(n) per call in V8). Now O(V+E); behavior is unchanged (same topological levels, same cycle detection). Fixes #307. diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index 6cd94e8a2..584fdb0e0 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/get-shit-done/bin/lib/phase.cjs @@ -353,6 +353,64 @@ function extractObjective(content) { return m ? m[1].trim() : null; } +// O(V + E). Assigns each in-phase plan its longest-path topological level over the +// in-phase dependsOn DAG (Kahn's algorithm). Returns { level: Map, visited: number }. +// visited < rawPlans.length signals a dependency cycle. +function computeDependencyLevels(rawPlans, planMap, canonicalToId) { + // Kahn's algorithm — compute in-degree and adjacency for in-phase deps only. + const level = new Map(); + const inDeg = new Map(); + const adj = new Map(); + + for (const p of rawPlans) { + if (!inDeg.has(p.id)) inDeg.set(p.id, 0); + if (!adj.has(p.id)) adj.set(p.id, []); + for (const dep of p.dependsOn) { + // Accept both full-stem ('03-01-auth-hardening') and canonical-prefix ('03-01') forms. + // All lookups are lowercased so mixed-case depends_on refs resolve correctly (#3785). + const depLower = dep.toLowerCase(); + const resolvedDep = planMap.has(depLower) ? planMap.get(depLower).id : canonicalToId.get(depLower); + if (!resolvedDep) continue; // external dep — ignore + if (!adj.has(resolvedDep)) adj.set(resolvedDep, []); + adj.get(resolvedDep).push(p.id); + inDeg.set(p.id, (inDeg.get(p.id) ?? 0) + 1); + } + } + + // Start with nodes that have no in-phase dependencies. + const queue = []; + for (const p of rawPlans) { + if ((inDeg.get(p.id) ?? 0) === 0) { + queue.push(p.id); + level.set(p.id, 0); + } + } + + // Dequeue by head index (queue[head++]), NOT Array.shift(): shift() is O(n) per + // call in V8 (it re-indexes the backing store), which would make this Kahn's BFS + // O(V^2) on deep queues (e.g. wide fan-in graphs). Head-index dequeue is O(1) + // amortized -> O(V+E) overall. Do not "simplify" this back to queue.shift(). (#307) + let head = 0; + let visited = 0; + while (head < queue.length) { + const cur = queue[head++]; + visited++; + const curLevel = level.get(cur); + for (const dep of (adj.get(cur) ?? [])) { + const newLevel = curLevel + 1; + if (newLevel > (level.get(dep) ?? -1)) { + level.set(dep, newLevel); + } + inDeg.set(dep, inDeg.get(dep) - 1); + if (inDeg.get(dep) === 0) { + queue.push(dep); + } + } + } + + return { level, visited }; +} + function cmdPhasePlanIndex(cwd, phase, raw) { if (!phase) { error('phase required for phase-plan-index'); @@ -496,51 +554,7 @@ function cmdPhasePlanIndex(cwd, phase, raw) { // like '01' or '01A'. Adding the third tier here is tracked as a parity // gap and is out of scope for #3785 / PR #3798. - // Kahn's algorithm — compute in-degree and adjacency for in-phase deps only. - const level = new Map(); - const inDeg = new Map(); - const adj = new Map(); - - for (const p of rawPlans) { - if (!inDeg.has(p.id)) inDeg.set(p.id, 0); - if (!adj.has(p.id)) adj.set(p.id, []); - for (const dep of p.dependsOn) { - // Accept both full-stem ('03-01-auth-hardening') and canonical-prefix ('03-01') forms. - // All lookups are lowercased so mixed-case depends_on refs resolve correctly (#3785). - const depLower = dep.toLowerCase(); - const resolvedDep = planMap.has(depLower) ? planMap.get(depLower).id : canonicalToId.get(depLower); - if (!resolvedDep) continue; // external dep — ignore - if (!adj.has(resolvedDep)) adj.set(resolvedDep, []); - adj.get(resolvedDep).push(p.id); - inDeg.set(p.id, (inDeg.get(p.id) ?? 0) + 1); - } - } - - // Start with nodes that have no in-phase dependencies. - const queue = []; - for (const p of rawPlans) { - if ((inDeg.get(p.id) ?? 0) === 0) { - queue.push(p.id); - level.set(p.id, 0); - } - } - - let visited = 0; - while (queue.length > 0) { - const cur = queue.shift(); - visited++; - const curLevel = level.get(cur); - for (const dep of (adj.get(cur) ?? [])) { - const newLevel = curLevel + 1; - if (newLevel > (level.get(dep) ?? -1)) { - level.set(dep, newLevel); - } - inDeg.set(dep, inDeg.get(dep) - 1); - if (inDeg.get(dep) === 0) { - queue.push(dep); - } - } - } + const { level, visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); // Cycle detection — any node not visited has a cycle. if (visited < rawPlans.length) { @@ -1555,4 +1569,5 @@ module.exports = { cmdPhaseInsert, cmdPhaseRemove, cmdPhaseComplete, + computeDependencyLevels, }; diff --git a/tests/phase-dependency-levels.test.cjs b/tests/phase-dependency-levels.test.cjs new file mode 100644 index 000000000..6354b16d8 --- /dev/null +++ b/tests/phase-dependency-levels.test.cjs @@ -0,0 +1,154 @@ +// allow-test-rule: source-text-is-the-product +// Tests for the extracted computeDependencyLevels pure function in phase.cjs. +// Covers correctness (behavior) and edge cases. The O(V+E) complexity contract +// is documented inline in computeDependencyLevels (phase.cjs) above the head-index +// queue loop; timing-based guards were removed (#307) because the O(V+E) +// Map-build constant dilutes the O(V^2) signal until N is ~1e6, making +// empirical ratio tests inherently flaky on contended CI runners. + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { computeDependencyLevels } = require('../get-shit-done/bin/lib/phase.cjs'); + +// Helper: build rawPlans + planMap + canonicalToId from a simple spec. +// spec is an array of { id, dependsOn } objects. +function buildInputs(spec) { + const rawPlans = spec.map(s => ({ id: s.id, dependsOn: s.dependsOn ?? [] })); + const planMap = new Map(rawPlans.map(p => [p.id.toLowerCase(), p])); + const canonicalToId = new Map(rawPlans.map(p => [p.id.toLowerCase(), p.id])); + return { rawPlans, planMap, canonicalToId }; +} + +describe('computeDependencyLevels — behavior tests', () => { + + // (a) Linear chain: 0 ← 1 ← 2 (1 depends on 0, 2 depends on 1) + test('(a) linear chain assigns levels 0,1,2 and visits all nodes', () => { + const { rawPlans, planMap, canonicalToId } = buildInputs([ + { id: 'p0', dependsOn: [] }, + { id: 'p1', dependsOn: ['p0'] }, + { id: 'p2', dependsOn: ['p1'] }, + ]); + const { level, visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + assert.equal(visited, 3); + assert.equal(level.get('p0'), 0); + assert.equal(level.get('p1'), 1); + assert.equal(level.get('p2'), 2); + }); + + // (b) Diamond: A; B,C depend on A; D depends on B and C → longest-path levels + // A=0, B=1, C=1, D=2 + test('(b) diamond uses longest-path (D=2, not 1)', () => { + const { rawPlans, planMap, canonicalToId } = buildInputs([ + { id: 'A', dependsOn: [] }, + { id: 'B', dependsOn: ['A'] }, + { id: 'C', dependsOn: ['A'] }, + { id: 'D', dependsOn: ['B', 'C'] }, + ]); + const { level, visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + assert.equal(visited, 4); + assert.equal(level.get('A'), 0); + assert.equal(level.get('B'), 1); + assert.equal(level.get('C'), 1); + assert.equal(level.get('D'), 2); + }); + + // (c) Independent set: no deps → all level 0 + test('(c) independent set: all nodes at level 0, all visited', () => { + const N = 10; + const spec = Array.from({ length: N }, (_, i) => ({ id: `node-${i}`, dependsOn: [] })); + const { rawPlans, planMap, canonicalToId } = buildInputs(spec); + const { level, visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + assert.equal(visited, N); + for (let i = 0; i < N; i++) { + assert.equal(level.get(`node-${i}`), 0, `node-${i} should be level 0`); + } + }); + + // (d) Cycle: A depends on B and B depends on A → visited < N + test('(d) cycle: visited < N signals cycle (not all nodes reachable)', () => { + const { rawPlans, planMap, canonicalToId } = buildInputs([ + { id: 'A', dependsOn: ['B'] }, + { id: 'B', dependsOn: ['A'] }, + ]); + const { visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + assert.ok(visited < rawPlans.length, `expected visited < 2, got ${visited}`); + }); + + // (e) Prefix resolution via canonicalToId: dep given as canonical prefix resolves + // to the full plan ID. Mirrors the #3785 behavior. + test('(e) canonical prefix resolution (depends_on short form resolves via canonicalToId)', () => { + // Plan with full stem ID; dep references canonical prefix only + const rawPlans = [ + { id: '03-01-auth-hardening', dependsOn: [] }, + { id: '03-02-token-rotation', dependsOn: ['03-01'] }, // short prefix dep + ]; + // planMap uses full-stem lowercase keys (planMap.get('03-01') would miss) + const planMap = new Map(rawPlans.map(p => [p.id.toLowerCase(), p])); + // canonicalToId maps prefix → full ID + const canonicalToId = new Map([ + ['03-01', '03-01-auth-hardening'], + ['03-02', '03-02-token-rotation'], + ]); + const { level, visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + assert.equal(visited, 2, 'both plans should be visited (no cycle)'); + assert.equal(level.get('03-01-auth-hardening'), 0); + assert.equal(level.get('03-02-token-rotation'), 1, 'prefix dep resolved → level 1'); + }); + + // (f) EMPTY: no plans → level.size === 0 and visited === 0, no throw. + test('(f) empty rawPlans: returns empty level map and visited === 0', () => { + const { level, visited } = computeDependencyLevels([], new Map(), new Map()); + assert.equal(level.size, 0, 'level map should be empty'); + assert.equal(visited, 0, 'visited should be 0 with no plans'); + }); + + // (g) SELF-LOOP: a plan whose dependsOn includes its own id → in-degree is never + // decremented to 0 → the plan is never enqueued → visited === 0 (cycle signalled). + // Self-dep: inDeg starts at 0, then gets +1 for the self-edge → inDeg = 1 forever. + test('(g) self-loop: plan is never enqueued, visited === 0 (cycle signalled)', () => { + const { rawPlans, planMap, canonicalToId } = buildInputs([ + { id: 'solo', dependsOn: ['solo'] }, + ]); + const { level, visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + assert.equal(visited, 0, 'self-loop plan should never be visited'); + assert.ok(visited < rawPlans.length, 'visited < rawPlans.length signals cycle'); + assert.ok(!level.has('solo'), 'self-loop plan should not appear in level map'); + }); + + // (h) DUPLICATE EDGE: plan B lists the same dep A twice. + // Effect: inDeg(B) = 2 (double-counted), adj(A) = ['B', 'B'] (double-pushed). + // When A is processed (curLevel=0): + // First 'B': inDeg(B) → 1, level(B) set to 1 (not pushed yet). + // Second 'B': inDeg(B) → 0, level(B) stays 1 (already set), B pushed. + // Result: visited === 2, level(A) === 0, level(B) === 1. + // This documents the existing behavior: duplicate deps double-count in-degree and + // double-push the adjacency list, but the final level/visited result is still correct + // because each decrement pairs with a push. + test('(h) duplicate edge: B lists dep A twice → visited === 2, levels A=0 B=1', () => { + const { rawPlans, planMap, canonicalToId } = buildInputs([ + { id: 'A', dependsOn: [] }, + { id: 'B', dependsOn: ['A', 'A'] }, // duplicate dep + ]); + const { level, visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + assert.equal(visited, 2, 'both A and B should be visited'); + assert.equal(level.get('A'), 0, 'A has no deps → level 0'); + assert.equal(level.get('B'), 1, 'B depends on A → level 1'); + }); + + // (i) EXTERNAL/UNRESOLVED DEP: plan B lists a dep that is neither in planMap nor in + // canonicalToId → the dep is ignored (continue), B retains in-degree 0 → B is level 0. + // Both A and B have in-degree 0 and are visited → visited === 2. + test('(i) unresolved/external dep is ignored: B still enqueued at level 0', () => { + const { rawPlans, planMap, canonicalToId } = buildInputs([ + { id: 'A', dependsOn: [] }, + { id: 'B', dependsOn: ['nonexistent-external-plan'] }, + ]); + const { level, visited } = computeDependencyLevels(rawPlans, planMap, canonicalToId); + assert.equal(visited, 2, 'both plans should be visited (external dep ignored)'); + assert.equal(level.get('A'), 0, 'A has no deps → level 0'); + assert.equal(level.get('B'), 0, 'B external dep ignored → in-degree 0 → level 0'); + }); + +});