Remove build_plan from lockfile to improve perf #1362
Conversation
|
This is a breaking change, in that |
|
Code looks good, though 👍 |
fsoikin
left a comment
fsoikin
left a comment
There was a problem hiding this comment.
There is also a comment that refers to storing all transitive dependencies in the lockfile:
spago/src/Spago/Command/Fetch.purs
Lines 162 to 164 in 782311b
I also noticed that there is a copy&paste of computing transitive deps for all workspace packages - here and here. Perhaps these should be consolidated?
| getDeps :: PackageName -> Set PackageName | ||
| getDeps name = case Map.lookup name lockfile.workspace.packages of | ||
| Just pkg -> Set.fromFoldable $ Map.keys $ unwrap pkg.core.dependencies | ||
| Nothing -> Set.fromFoldable $ case Map.lookup name lockfile.packages of | ||
| Just (FromPath { dependencies }) -> dependencies | ||
| Just (FromGit { dependencies }) -> dependencies | ||
| Just (FromRegistry { dependencies }) -> dependencies | ||
| Nothing -> [] | ||
|
|
||
| transitiveClosure :: Set PackageName -> Set PackageName | ||
| transitiveClosure initial = go initial initial | ||
| where | ||
| allWorkspacePackages = Map.fromFoldable $ map (\p -> Tuple p.package.name (WorkspacePackage p)) (Config.getWorkspacePackages workspace.packageSet) | ||
|
|
||
| isInBuildPlan :: forall v. PackageName -> v -> Boolean | ||
| isInBuildPlan name _package = Set.member name bp | ||
|
|
||
| workspacePackagesWeNeed = Map.filterWithKey isInBuildPlan allWorkspacePackages | ||
| otherPackages = map fromLockEntry $ Map.filterWithKey isInBuildPlan lockfile.packages | ||
| go seen new | ||
| | Set.isEmpty new = seen | ||
| | otherwise = | ||
| let | ||
| discovered = foldMap getDeps new `Set.difference` seen | ||
| in | ||
| go (seen <> discovered) discovered | ||
|
|
||
| computeTransitiveDeps :: Dependencies -> PackageMap | ||
| computeTransitiveDeps deps = | ||
| let | ||
| transitiveDeps = transitiveClosure $ Set.fromFoldable $ Map.keys $ unwrap deps | ||
|
|
||
| isInTransitiveDeps :: forall v. PackageName -> v -> Boolean | ||
| isInTransitiveDeps name _ = Set.member name transitiveDeps | ||
| in | ||
| Map.union | ||
| (Map.filterWithKey isInTransitiveDeps allWorkspacePackages) | ||
| (map fromLockEntry $ Map.filterWithKey isInTransitiveDeps lockfile.packages) |
There was a problem hiding this comment.
I don't quite understand why all of this is necessary.
Before this change, the transitive dependencies used to be initially calculated in the Left branch of this case, and then written to the lockfile, and subsequently lifted from there. This suggests that the logic for computing them already exists in some form inside the Left branch.
There was a problem hiding this comment.
It does, and I tried to unify it, but I now think that the two logics are different enough that it's not worth unifying them: the algo in question is in the Left branch and only in the case of a package set build (since otherwise the solver build is using the solver to get the transitive closure, which is different logic).
The overall algorithm is the same, but when solving a package set build one has effects, caches, and error reporting (since the plan build might fail).
We have none of that when we build a plan from the lockfile, so we can get away with much simpler implementation, even though the logic is overall the same.
Co-authored-by: Fyodor Soikin <name.fa@gmail.com>
Fix #1262 by following the approach I explained at the time - we remove
build_planfrom each package of the lockfile since it's faster to recompute it than to decode it, as the files can get massive.cc @finnhodgkin