Rewrite dependency resolution using algebraic-graphs#2
Merged
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>
ocharles
changed the title
ocharles
changed the title
ocharles
changed the title
algebraic-graphs
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.
We know that
dbmigrationshas a horrific slowdown with non-linear migration graphs. I've slapped Claude on it and generated a pathological test case in - this takes close to a minute onmaster, but with this rewrite it's in the order of seconds.