Rewrite dependency resolution on algebraic-graphs (fixes exponential non-linear DAG slowdown)#2
Open
ocharles wants to merge 2 commits into
Open
Rewrite dependency resolution on algebraic-graphs (fixes exponential non-linear DAG slowdown)#2ocharles wants to merge 2 commits into
ocharles wants to merge 2 commits into
Conversation
dependenciesWith enumerated every path from the start object (each object's direct successor names followed by the recursive expansion of each name), then deduplicated the result with the quadratic cleanLDups. The number of paths is exponential in the depth of a non-linear dependency graph, so an upgrade over a "diamond ladder" of just 46 migrations took ~55 seconds. Produce the identical ordering in linear time by scanning the virtual path enumeration from the end: walk successor names in reverse order, expand each name's subtree at most once (a repeated expansion occurs earlier in the enumeration, so it cannot contain a last occurrence), and emit each name the first time it is seen, i.e. at its last occurrence. Like the old code, the traversal is name-driven rather than node-driven, so behaviour is preserved even for graphs with duplicate object names. Also replace the assoc-list lookup in dependenciesWith and the list-membership filters in migrationsToApply/migrationsToRevert with Map/Set equivalents. Add a stress test that simulates the upgrade command over a diamond-ladder graph (two migrations per level, each depending on both migrations of the previous level). It now runs in ~3ms where it previously took ~55s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the fgl-based dependency graph and the hand-rolled traversal and cycle detection with algebraic-graphs, which postdates this library's original design: - The graph is now an AdjacencyMap keyed by object name, built directly with `stars`, so the node-index bookkeeping (depGraphObjectMap/depGraphNameMap) disappears. - dependencies/reverseDependencies are `reachable` + `induce` + `topSort` (reverseDependencies via `transpose`). topSort returns the lexicographically smallest topological ordering, so results are deterministic by construction rather than dependent on graph node numbering (i.e. on directory listing order). - The CycleDetection module is deleted: cycle rejection now comes from `topSort`'s Left result, which contains the actual cycle, so mkDepGraph can finally report which objects form the cycle (resolving the long-standing XXX comment). Its test cases are ported to mkDepGraph-level tests. - mkDepGraph now rejects duplicate object identifiers explicitly. Previously duplicates were silently tolerated with confusing behaviour (name lookups resolved to the first match while the graph contained both vertices). Ordering semantics: any result is still a valid topological order, but the specific order changes where siblings were previously ordered by node number; one DependencyTest expectation is updated accordingly. Linear migration histories are unaffected (their topological order is unique). The non-linear stress test runs marginally faster than the previous implementation (46 migrations: 2.3ms; 2002 migrations: 7.4s vs 9.8s). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Alternative to (and superset of) #1 — this branch contains that PR's commit plus a rewrite of dependency resolution on top of
algebraic-graphs, which postdates this library's original design. Merging this makes #1 redundant.Problem
Same as #1:
dependenciesWithenumerated every path through the dependency graph and deduplicated with the quadraticcleanLDups, which is exponential in the depth of a non-linear DAG.upgraderuns this once per missing migration, so any non-linear migration history becomes unusably slow — forcing migration histories to be kept fully serialized.Fix
Two commits:
test/NonLinearStressTest.hs), which simulates theupgradecommand over a "diamond ladder" (two migrations per level, each depending on both migrations of the previous level) and asserts correctness + a 10s time limit.Database.Schema.Migrations.Dependenciesonalgebraic-graphs(net −133 lines):AdjacencyMap Textkeyed by migration name, built withstars; all fgl node-index bookkeeping (depGraphObjectMap/depGraphNameMap) disappears, andfglis dropped as a dependency.dependencies/reverseDependenciesarereachable+induce+topSort(the reverse direction viatranspose) — exponential blowup is impossible by construction.CycleDetection(hand-rolled tricolor DFS over assoc lists) is deleted:topSortreturnsLeftwith the actual cycle, somkDepGraphfinally reports which migrations form the cycle, resolving the long-standingXXXcomment. Its test cases are ported tomkDepGraph-level tests.mkDepGraphnow rejects duplicate object identifiers explicitly (previously silently tolerated, with name lookups resolving to the first match while the graph contained both vertices).Performance
Full simulated upgrade of the diamond ladder:
The residual superlinear growth at implausibly large store sizes is
upgraderecomputing an O(n) plan once per missing migration — inherent to the command's shape.Ordering semantics
topSortreturns the lexicographically smallest topological ordering, so apply/revert order is now documented-deterministic rather than an artifact of graph node numbering (i.e. directory listing order). Any result is still a valid topological order, but where independent sibling migrations were previously ordered by node number they are now ordered by name; exactly oneDependencyTestexpectation changed accordingly (verified by hand to be a valid revert order). Linear migration histories are unaffected — their topological order is unique — so this changes nothing for a fully serialized store like ours.This ordering change is the difference from #1, which reproduced the legacy ordering exactly. If we want zero observable change, merge #1 alone; if we're happy adopting deterministic lexicographic ordering (recommended), merge this and close #1.
🤖 Generated with Claude Code