Skip to content

Commit

Permalink
Return and track affected and culprit on conflicts
Browse files Browse the repository at this point in the history
Whenever we either discard a version due to its dependencies or perform conflict resolution, we return the last conflict that led to discarding them.

In cargo, we use this information for prioritization, which speeds up resolution (`cargo run -r -- -m pub --with-solana --filter solana-archiver-lib -t 16` goes from 90s to 20s on my machine).

Configurations that are noticeably slower for the solana test case:
* All incompatibilities unit propagation
* Only the last root cause in unit propagation
* No incompatibilities from unit propagation
* No incompatibilities from `add_version`
* Only affect counts (without culprit counts)
* Backtracking with the same heuristic as astral-sh/uv#9843 (backtracking once after affected hits 5)

In uv, we use this to re-prioritize and backtrack when a package decision accumulated to many conflicts. Since we have our own solver loop, we add the incompatibility to our own tracking instead.

Built on #291

## Benchmarks

Main:

```
        index commit hash: 82086e46740d7a9303216bfac093e7268a95121f
        index commit time: 2024-11-30T18:18:14Z
               index size: 32
          solana in index: 32
             Pub CPU time:  1215.49s ==  20.26min
           Cargo CPU time: skipped
Cargo check lock CPU time: skipped
  Pub check lock CPU time: skipped
                Wall time:    80.58s ==   1.34min
```

With #291:

```
        index commit hash: 82086e46740d7a9303216bfac093e7268a95121f
        index commit time: 2024-11-30T18:18:14Z
               index size: 32
          solana in index: 32
             Pub CPU time:   467.73s ==   7.80min
           Cargo CPU time: skipped
Cargo check lock CPU time: skipped
  Pub check lock CPU time: skipped
                Wall time:    34.76s ==   0.58min
```

This PR:

```
        index commit hash: 82086e46740d7a9303216bfac093e7268a95121f
        index commit time: 2024-11-30T18:18:14Z
               index size: 32
          solana in index: 32
             Pub CPU time:   271.79s ==   4.53min
           Cargo CPU time: skipped
Cargo check lock CPU time: skipped
  Pub check lock CPU time: skipped
                Wall time:    20.17s ==   0.34min
```
  • Loading branch information
konstin committed Dec 16, 2024
1 parent e9170cb commit d839d70
Show file tree
Hide file tree
Showing 7 changed files with 168 additions and 51 deletions.
15 changes: 12 additions & 3 deletions examples/caching_dependency_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

use std::cell::RefCell;

use pubgrub::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider, Ranges};
use pubgrub::{
resolve, Dependencies, DependencyProvider, OfflineDependencyProvider,
PackageResolutionStatistics, Ranges,
};

type NumVS = Ranges<u32>;

Expand Down Expand Up @@ -57,8 +60,14 @@ impl<DP: DependencyProvider<M = String>> DependencyProvider for CachingDependenc

type Priority = DP::Priority;

fn prioritize(&self, package: &DP::P, ranges: &DP::VS) -> Self::Priority {
self.remote_dependencies.prioritize(package, ranges)
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_statistics: &PackageResolutionStatistics,
) -> Self::Priority {
self.remote_dependencies
.prioritize(package, range, package_statistics)
}

type Err = DP::Err;
Expand Down
10 changes: 8 additions & 2 deletions src/internal/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,16 @@ impl<DP: DependencyProvider> State<DP> {

/// Unit propagation is the core mechanism of the solving algorithm.
/// CF <https://github.com/dart-lang/pub/blob/master/doc/solver.md#unit-propagation>
///
/// For each package with a satisfied incompatibility, returns the package and the root cause
/// incompatibility.
#[cold]
#[allow(clippy::type_complexity)] // Type definitions don't support impl trait.
pub(crate) fn unit_propagation(
&mut self,
package: Id<DP::P>,
) -> Result<(), NoSolutionError<DP>> {
) -> Result<Vec<(Id<DP::P>, IncompDpId<DP>)>, NoSolutionError<DP>> {
let mut root_causes = Vec::new();
self.unit_propagation_buffer.clear();
self.unit_propagation_buffer.push(package);
while let Some(current_package) = self.unit_propagation_buffer.pop() {
Expand Down Expand Up @@ -168,6 +173,7 @@ impl<DP: DependencyProvider> State<DP> {
.map_err(|terminal_incompat_id| {
self.build_derivation_tree(terminal_incompat_id)
})?;
root_causes.push((package, root_cause));
self.unit_propagation_buffer.clear();
self.unit_propagation_buffer.push(package_almost);
// Add to the partial solution with incompat as cause.
Expand All @@ -183,7 +189,7 @@ impl<DP: DependencyProvider> State<DP> {
}
}
// If there are no more changed packages, unit propagation is done.
Ok(())
Ok(root_causes)
}

/// Return the root cause or the terminal incompatibility.
Expand Down
67 changes: 43 additions & 24 deletions src/internal/partial_solution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
#[cold]
pub(crate) fn pick_highest_priority_pkg(
&mut self,
prioritizer: impl Fn(Id<DP::P>, &DP::VS) -> DP::Priority,
mut prioritizer: impl FnMut(Id<DP::P>, &DP::VS) -> DP::Priority,
) -> Option<Id<DP::P>> {
let check_all = self.changed_this_decision_level
== self.current_decision_level.0.saturating_sub(1) as usize;
Expand Down Expand Up @@ -304,7 +304,21 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
.map(|(&p, pa)| match &pa.assignments_intersection {
AssignmentsIntersection::Decision((_, v, _)) => (p, v.clone()),
AssignmentsIntersection::Derivations(_) => {
panic!("Derivations in the Decision part")
let mut context = String::new();
for (id, assignment) in self
.package_assignments
.iter()
.take(self.current_decision_level.0 as usize)
{
context.push_str(&format!(
" * {:?} {:?}\n",
id, assignment.assignments_intersection
));
}
panic!(
"Derivations in the Decision part. Decision level {}\n{}",
self.current_decision_level.0, context
)
}
})
}
Expand Down Expand Up @@ -362,34 +376,39 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
version: DP::V,
new_incompatibilities: std::ops::Range<IncompId<DP::P, DP::VS, DP::M>>,
store: &Arena<Incompatibility<DP::P, DP::VS, DP::M>>,
) {
) -> Option<IncompId<DP::P, DP::VS, DP::M>> {
if !self.has_ever_backtracked {
// Nothing has yet gone wrong during this resolution. This call is unlikely to be the first problem.
// Fast path: Nothing has yet gone wrong during this resolution. This call is unlikely to be the first problem.
// So let's live with a little bit of risk and add the decision without checking the dependencies.
// The worst that can happen is we will have to do a full backtrack which only removes this one decision.
log::info!("add_decision: {package:?} @ {version} without checking dependencies");
self.add_decision(package, version);
return None;
}

// Check if any of the dependencies preclude deciding on this crate version.
let package_term = Term::exact(version.clone());
let relation = |incompat: IncompId<DP::P, DP::VS, DP::M>| {
store[incompat].relation(|p| {
// The current package isn't part of the package assignments yet.
if p == package {
Some(&package_term)
} else {
self.term_intersection_for_package(p)
}
})
};
if let Some(satisfied) = Id::range_to_iter(new_incompatibilities)
.find(|incompat| relation(*incompat) == Relation::Satisfied)
{
log::info!(
"rejecting decision {package:?} @ {version} because its dependencies conflict"
);
Some(satisfied)
} else {
// Check if any of the new dependencies preclude deciding on this crate version.
let exact = Term::exact(version.clone());
let not_satisfied = |incompat: &Incompatibility<DP::P, DP::VS, DP::M>| {
incompat.relation(|p| {
if p == package {
Some(&exact)
} else {
self.term_intersection_for_package(p)
}
}) != Relation::Satisfied
};

// Check none of the dependencies (new_incompatibilities)
// would create a conflict (be satisfied).
if store[new_incompatibilities].iter().all(not_satisfied) {
log::info!("add_decision: {package:?} @ {version}");
self.add_decision(package, version);
} else {
log::info!("not adding {package:?} @ {version} because of its dependencies",);
}
log::info!("adding decision: {package:?} @ {version}");
self.add_decision(package, version);
None
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
//! and [SemanticVersion] for versions.
//! This may be done quite easily by implementing the three following functions.
//! ```
//! # use pubgrub::{DependencyProvider, Dependencies, SemanticVersion, Ranges, DependencyConstraints, Map};
//! # use pubgrub::{DependencyProvider, Dependencies, SemanticVersion, Ranges, DependencyConstraints, Map, PackageResolutionStatistics};
//! # use std::error::Error;
//! # use std::borrow::Borrow;
//! # use std::convert::Infallible;
Expand All @@ -86,7 +86,7 @@
//! }
//!
//! type Priority = usize;
//! fn prioritize(&self, package: &String, range: &SemVS) -> Self::Priority {
//! fn prioritize(&self, package: &String, range: &SemVS, conflicts_counts: &PackageResolutionStatistics) -> Self::Priority {
//! unimplemented!()
//! }
//!
Expand Down Expand Up @@ -227,7 +227,7 @@ pub use report::{
DefaultStringReportFormatter, DefaultStringReporter, DerivationTree, Derived, External,
ReportFormatter, Reporter,
};
pub use solver::{resolve, Dependencies, DependencyProvider};
pub use solver::{resolve, Dependencies, DependencyProvider, PackageResolutionStatistics};
pub use term::Term;
pub use type_aliases::{DependencyConstraints, Map, SelectedDependencies, Set};
pub use version::{SemanticVersion, VersionParseError};
Expand Down
23 changes: 16 additions & 7 deletions src/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::cmp::Reverse;
use std::collections::BTreeMap;
use std::convert::Infallible;

use crate::solver::PackageResolutionStatistics;
use crate::{Dependencies, DependencyConstraints, DependencyProvider, Map, Package, VersionSet};

/// A basic implementation of [DependencyProvider].
Expand Down Expand Up @@ -92,15 +93,23 @@ impl<P: Package, VS: VersionSet> DependencyProvider for OfflineDependencyProvide
.and_then(|versions| versions.keys().rev().find(|v| range.contains(v)).cloned()))
}

type Priority = Reverse<usize>;
type Priority = (u32, Reverse<usize>);

#[inline]
fn prioritize(&self, package: &P, range: &VS) -> Self::Priority {
Reverse(
self.dependencies
.get(package)
.map(|versions| versions.keys().filter(|v| range.contains(v)).count())
.unwrap_or(0),
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_statistics: &PackageResolutionStatistics,
) -> Self::Priority {
(
package_statistics.conflict_count(),
Reverse(
self.dependencies
.get(package)
.map(|versions| versions.keys().filter(|v| range.contains(v)).count())
.unwrap_or(0),
),
)
}

Expand Down
76 changes: 70 additions & 6 deletions src/solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,31 @@ use log::{debug, info};
use crate::internal::{Id, Incompatibility, State};
use crate::{DependencyConstraints, Map, Package, PubGrubError, SelectedDependencies, VersionSet};

/// Statistics on how often a package conflicted with other packages.
#[derive(Debug, Default, Clone)]
pub struct PackageResolutionStatistics {
unit_propagation_affected: u32,
unit_propagation_culprit: u32,
dependencies_affected: u32,
dependencies_culprit: u32,
}

impl PackageResolutionStatistics {
/// The number of conflicts this package was involved in.
///
/// Processing packages with a high conflict count earlier usually speeds up resolution.
///
/// Whenever a package is part of the root cause incompatibility of a conflict, we increase its
/// count by one. Since the structure of the incompatibilities may change, this count too may
/// change in the future.
pub fn conflict_count(&self) -> u32 {
self.unit_propagation_affected
+ self.unit_propagation_culprit
+ self.dependencies_affected
+ self.dependencies_culprit
}
}

/// Main function of the library.
/// Finds a set of packages satisfying dependency bounds for a given package + version pair.
#[cold]
Expand All @@ -77,6 +102,7 @@ pub fn resolve<DP: DependencyProvider>(
version: impl Into<DP::V>,
) -> Result<SelectedDependencies<DP>, PubGrubError<DP>> {
let mut state: State<DP> = State::init(package.clone(), version.into());
let mut conflict_tracker: Map<Id<DP::P>, PackageResolutionStatistics> = Map::default();
let mut added_dependencies: Map<Id<DP::P>, Set<DP::V>> = Map::default();
let mut next = state.root_package;
loop {
Expand All @@ -88,7 +114,22 @@ pub fn resolve<DP: DependencyProvider>(
"unit_propagation: {:?} = '{}'",
&next, state.package_store[next]
);
state.unit_propagation(next)?;
let root_causes = state.unit_propagation(next)?;
for (affected, incompat) in root_causes {
conflict_tracker
.entry(affected)
.or_default()
.unit_propagation_affected += 1;
for (conflict_package, _) in state.incompatibility_store[incompat].iter() {
if conflict_package == affected {
continue;
}
conflict_tracker
.entry(conflict_package)
.or_default()
.unit_propagation_culprit += 1;
}
}

debug!(
"Partial solution after unit propagation: {}",
Expand All @@ -97,7 +138,11 @@ pub fn resolve<DP: DependencyProvider>(

let Some(highest_priority_pkg) =
state.partial_solution.pick_highest_priority_pkg(|p, r| {
dependency_provider.prioritize(&state.package_store[p], r)
dependency_provider.prioritize(
&state.package_store[p],
r,
conflict_tracker.entry(p).or_default(),
)
})
else {
return Ok(state
Expand Down Expand Up @@ -174,9 +219,23 @@ pub fn resolve<DP: DependencyProvider>(
let dep_incompats =
state.add_incompatibility_from_dependencies(p, v.clone(), dependencies);

state
.partial_solution
.add_version(p, v, dep_incompats, &state.incompatibility_store);
if let Some(conflict) = state.partial_solution.add_version(
p,
v,
dep_incompats,
&state.incompatibility_store,
) {
conflict_tracker.entry(p).or_default().dependencies_affected += 1;
for (incompat_package, _) in state.incompatibility_store[conflict].iter() {
if incompat_package == p {
continue;
}
conflict_tracker
.entry(incompat_package)
.or_default()
.unit_propagation_culprit += 1;
}
}
} else {
// `dep_incompats` are already in `incompatibilities` so we know there are not satisfied
// terms and can add the decision directly.
Expand Down Expand Up @@ -254,7 +313,12 @@ pub trait DependencyProvider {
///
/// Note: the resolver may call this even when the range has not changed,
/// if it is more efficient for the resolvers internal data structures.
fn prioritize(&self, package: &Self::P, range: &Self::VS) -> Self::Priority;
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_conflicts_counts: &PackageResolutionStatistics,
) -> Self::Priority;
/// The type returned from `prioritize`. The resolver does not care what type this is
/// as long as it can pick a largest one and clone it.
///
Expand Down
22 changes: 16 additions & 6 deletions tests/proptest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ use proptest::string::string_regex;

use pubgrub::{
resolve, DefaultStringReporter, Dependencies, DependencyProvider, DerivationTree, External,
OfflineDependencyProvider, Package, PubGrubError, Ranges, Reporter, SelectedDependencies,
VersionSet,
OfflineDependencyProvider, Package, PackageResolutionStatistics, PubGrubError, Ranges,
Reporter, SelectedDependencies, VersionSet,
};

use crate::sat_dependency_provider::SatResolve;
Expand Down Expand Up @@ -49,8 +49,13 @@ impl<P: Package, VS: VersionSet> DependencyProvider for OldestVersionsDependency

type Priority = <OfflineDependencyProvider<P, VS> as DependencyProvider>::Priority;

fn prioritize(&self, package: &P, range: &VS) -> Self::Priority {
self.0.prioritize(package, range)
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_statistics: &PackageResolutionStatistics,
) -> Self::Priority {
self.0.prioritize(package, range, package_statistics)
}

type Err = Infallible;
Expand Down Expand Up @@ -104,8 +109,13 @@ impl<DP: DependencyProvider> DependencyProvider for TimeoutDependencyProvider<DP

type Priority = DP::Priority;

fn prioritize(&self, package: &DP::P, range: &DP::VS) -> Self::Priority {
self.dp.prioritize(package, range)
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_statistics: &PackageResolutionStatistics,
) -> Self::Priority {
self.dp.prioritize(package, range, package_statistics)
}

type Err = DP::Err;
Expand Down

0 comments on commit d839d70

Please sign in to comment.