Skip to content

Perf: Speed up solver #11566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
parsonsmatt wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Perf: Speed up solver #11566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86721c1
Speed up solver
parsonsmatt Mar 3, 2026
47e5847
Address review feedback
parsonsmatt Mar 4, 2026
85fe65e
Add a Set of QPN
parsonsmatt Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cabal-install-solver/src/Distribution/Solver/Modular/Assignment.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import qualified Distribution.Solver.Types.ComponentDeps as CD
import Distribution.Solver.Types.OptionalStanza
import Distribution.Solver.Types.PackagePath

import Distribution.Simple.Utils (ordNub)
import Distribution.Solver.Modular.Configured
import Distribution.Solver.Modular.Dependency
import Distribution.Solver.Modular.Flag
Expand Down Expand Up @@ -52,8 +53,8 @@ toCPs (A pa fa sa) rdm =
g :: Graph Component
vm :: Vertex -> ((), QPN, [(Component, QPN)])
cvm :: QPN -> Maybe Vertex
-- Note that the RevDepMap contains duplicate dependencies. Therefore the nub.
(g, vm, cvm) = graphFromEdges (L.map (\ (x, xs) -> ((), x, nub xs))
-- Note that the RevDepMap contains duplicate dependencies. Therefore the dedup.
(g, vm, cvm) = graphFromEdges (L.map (\ (x, xs) -> ((), x, ordNub xs))
(M.toList rdm))
tg :: Graph Component
tg = transposeG g
Expand Down
26 changes: 20 additions & 6 deletions cabal-install-solver/src/Distribution/Solver/Modular/ConflictSet.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{-# LANGUAGE PatternSynonyms #-}
-- | Conflict sets
--
-- Intended for double import
Expand All @@ -8,7 +9,7 @@ module Distribution.Solver.Modular.ConflictSet (
ConflictSet -- opaque
, Conflict(..)
, ConflictMap
, OrderedVersionRange(..)
, OrderedVersionRange(OrderedVersionRange)
, showConflictSet
, showCSSortedByFrequency
, showCSWithFrequency
Expand Down Expand Up @@ -40,6 +41,8 @@ import qualified Data.Set as S
import Distribution.Solver.Modular.Var
import Distribution.Solver.Modular.Version
import Distribution.Solver.Types.PackagePath
import Distribution.Utils.ShortText (ShortText)
import qualified Distribution.Utils.ShortText as ShortText

-- | The set of variables involved in a solver conflict, each paired with
-- details about the conflict.
Expand Down Expand Up @@ -78,13 +81,24 @@ data Conflict =
| OtherConflict
deriving (Eq, Ord, Show)

-- | Version range with an 'Ord' instance.
newtype OrderedVersionRange = OrderedVersionRange VR
deriving (Eq, Show)
-- | Version range with an 'Ord' instance. The show string is cached to avoid
-- recomputing it on every comparison.
data OrderedVersionRange = OVR !ShortText !VR
Comment on lines -81 to +86
Copy link
Collaborator

@sebright sebright Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like #8269 added an Ord instance to VersionRange, so OrderedVersionRange could be removed, unless it leads to a significant regression.

Copy link
Collaborator Author

@parsonsmatt parsonsmatt Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be surprised if it is a significant difference either way - compare on VersionRange may involve more pointer chasing, particularly on Union or Intersect cases, but compare @Version I'd expect to be very fast in the PV0 case, which may overcome the textual prefix comparison. Removing OrderedVersionRange feels more invasive than optimizing it transparently.


pattern OrderedVersionRange :: VR -> OrderedVersionRange
pattern OrderedVersionRange vr <- OVR _ vr
where OrderedVersionRange vr = OVR (ShortText.toShortText (show vr)) vr
{-# COMPLETE OrderedVersionRange #-}

instance Eq OrderedVersionRange where
OVR _ a == OVR _ b = a == b

-- TODO: Avoid converting the version ranges to strings.
instance Ord OrderedVersionRange where
compare = compare `on` show
Copy link
Collaborator Author

@parsonsmatt parsonsmatt Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funny - because we'd be doing "OrderedVersionRange ..." as the prefix of every string, a Set ORderedVersionRange would be a linked list for that prefix. This is delegating to show on the VR which may be similarly disadvantaged, but at least we're caching

Copy link
Collaborator

@geekosaur geekosaur Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be, but a drop 20 on the String would resolve that cheaply?

compare (OVR sa _) (OVR sb _) = compare sa sb

instance Show OrderedVersionRange where
showsPrec d (OVR _ vr) = showParen (d > 10) $
showString "OrderedVersionRange " . showsPrec 11 vr

showConflictSet :: ConflictSet -> String
showConflictSet = intercalate ", " . map showVar . toList
Expand Down
23 changes: 17 additions & 6 deletions cabal-install-solver/src/Distribution/Solver/Modular/Explore.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ exploreLog mbj enableBj fineGrainedConflicts (CountConflicts countConflicts) idx
, esBackjumps = 0
}

qo = defaultQualifyOptions idx

-- Is it possible for this package instance (QPN and POption) to resolve any
-- of the conflicts that were caused by the previous instance? The default
-- is true, because it is always safe to explore a package instance.
Expand All @@ -270,21 +272,30 @@ exploreLog mbj enableBj fineGrainedConflicts (CountConflicts countConflicts) idx
couldResolveConflicts :: QPN -> POption -> S.Set CS.Conflict -> Maybe ConflictSet
couldResolveConflicts currentQPN@(Q _ pn) (POption i@(I v _) _) conflicts =
let (PInfo deps _ _ _) = idx M.! pn M.! i
qdeps = qualifyDeps (defaultQualifyOptions idx) currentQPN deps
qdeps = qualifyDeps qo currentQPN deps

-- Pre-index: map from QPN to intersected version range (Constrained only)
depVRs :: M.Map QPN VR
depVRs = M.fromListWith (.&&.)
[ (qpn, case ci of Constrained vr -> vr; _ -> anyVersion)
| Simple (LDep _ (Dep (PkgComponent qpn _) ci)) _ <- qdeps
]
Comment on lines +277 to +282
Copy link
Collaborator

@sebright sebright Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This index should only contain dependencies that are Constrained now, since CS.VersionConstraintConflict doesn't handle the others.


depsWithVRs :: S.Set QPN
depsWithVRs = M.keysSet depVRs
Comment on lines +284 to +285
Copy link
Collaborator

@sebright sebright Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable can just be called deps now, because it doesn't matter whether the dependencies are constrained for resolving CS.GoalConflict.


couldBeResolved :: CS.Conflict -> Maybe ConflictSet
couldBeResolved CS.OtherConflict = Nothing
couldBeResolved (CS.GoalConflict conflictingDep) =
-- Check whether this package instance also has 'conflictingDep'
-- as a dependency (ignoring flag and stanza choices).
if null [() | Simple (LDep _ (Dep (PkgComponent qpn _) _)) _ <- qdeps, qpn == conflictingDep]
then Nothing
else Just CS.empty
if S.member conflictingDep depsWithVRs
then Just CS.empty
else Nothing
couldBeResolved (CS.VersionConstraintConflict dep excludedVersion) =
-- Check whether this package instance also excludes version
-- 'excludedVersion' of 'dep' (ignoring flag and stanza choices).
let vrs = [vr | Simple (LDep _ (Dep (PkgComponent qpn _) (Constrained vr))) _ <- qdeps, qpn == dep ]
vrIntersection = L.foldl' (.&&.) anyVersion vrs
let vrIntersection = M.findWithDefault anyVersion dep depVRs
in if checkVR vrIntersection excludedVersion
then Nothing
else -- If we skip this package instance, we need to update the
Expand Down
5 changes: 1 addition & 4 deletions cabal-install-solver/src/Distribution/Solver/Modular/Validate.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ checkComponentsInNewPackage :: ComponentDependencyReasons
-> Map ExposedComponent ComponentInfo
-> Either Conflict ()
checkComponentsInNewPackage required qpn providedComps =
case M.toList $ deleteKeys (M.keys providedComps) required of
case M.toList $ M.difference required providedComps of
Copy link
Collaborator Author

@parsonsmatt parsonsmatt Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be algorithmically better here

Copy link
Collaborator

@sebright sebright Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

(missingComp, dr) : _ ->
Left $ mkConflict missingComp dr NewPackageIsMissingRequiredComponent
[] ->
Expand All @@ -325,9 +325,6 @@ checkComponentsInNewPackage required qpn providedComps =
mkConflict comp dr mkFailure =
(CS.insert (P qpn) (dependencyReasonToConflictSet dr), mkFailure comp dr)

deleteKeys :: Ord k => [k] -> Map k v -> Map k v
deleteKeys ks m = L.foldr M.delete m ks

-- | We try to extract as many concrete dependencies from the given flagged
-- dependencies as possible. We make use of all the flag knowledge we have
-- already acquired.
Expand Down
Loading