Skip to content
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

Track outdated priorities in a set #313

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Track outdated priorities in a set #313

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e05640
Track outdated priorities in a set
konstin Jan 24, 2025
19c67d0
Remove failure branch with panic
konstin Jan 27, 2025
fa044b1
Update doc and comments
konstin Jan 29, 2025
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: 0 additions & 5 deletions src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ pub enum PubGrubError<DP: DependencyProvider> {
/// returned an error in the method [should_cancel](DependencyProvider::should_cancel).
#[error("We should cancel")]
ErrorInShouldCancel(#[source] DP::Err),

/// Something unexpected happened.
#[error("{0}")]
Failure(String),
}

impl<DP: DependencyProvider> From<NoSolutionError<DP>> for PubGrubError<DP> {
Expand Down Expand Up @@ -78,7 +74,6 @@ where
Self::ErrorInShouldCancel(arg0) => {
f.debug_tuple("ErrorInShouldCancel").field(arg0).finish()
}
Self::Failure(arg0) => f.debug_tuple("Failure").field(arg0).finish(),
}
}
}
87 changes: 44 additions & 43 deletions src/internal/partial_solution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::internal::{
use crate::{DependencyProvider, Package, Term, VersionSet};

type FnvIndexMap<K, V> = indexmap::IndexMap<K, V, BuildHasherDefault<FxHasher>>;
type FnvIndexSet<T> = indexmap::IndexSet<T, BuildHasherDefault<FxHasher>>;

#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)]
pub(crate) struct DecisionLevel(pub(crate) u32);
Expand Down Expand Up @@ -62,8 +63,6 @@ pub(crate) struct PartialSolution<DP: DependencyProvider> {
/// range.
Copy link
Member

Choose a reason for hiding this comment

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

The comet ending on this line is now out of date.

#[allow(clippy::type_complexity)]
package_assignments: FnvIndexMap<Id<DP::P>, PackageAssignments<DP::P, DP::VS, DP::M>>,
/// Index into `package_assignments` to decide which packages need to be re-prioritized.
prioritize_decision_level: usize,
/// The undecided packages order by their `Priority`.
///
/// The max heap allows quickly `pop`ing the highest priority package.
Expand All @@ -73,6 +72,9 @@ pub(crate) struct PartialSolution<DP: DependencyProvider> {
#[allow(clippy::type_complexity)]
prioritized_potential_packages:
PriorityQueue<Id<DP::P>, (DP::Priority, Reverse<u32>), BuildHasherDefault<FxHasher>>,
/// Packages whose derivations changed since the last time `prioritize` was called and need
/// their priorities to be updated.
outdated_priorities: FnvIndexSet<Id<DP::P>>,
/// Whether we have never backtracked, to enable fast path optimizations.
has_ever_backtracked: bool,
}
Expand Down Expand Up @@ -180,7 +182,7 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
current_decision_level: DecisionLevel(0),
package_assignments: FnvIndexMap::default(),
prioritized_potential_packages: PriorityQueue::default(),
prioritize_decision_level: 0,
outdated_priorities: FnvIndexSet::default(),
has_ever_backtracked: false,
}
}
Expand Down Expand Up @@ -233,10 +235,6 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
}
},
}
assert_eq!(
self.prioritize_decision_level,
self.package_assignments.len()
);
}
let new_idx = self.current_decision_level.0 as usize;
self.current_decision_level = self.current_decision_level.increment();
Expand Down Expand Up @@ -272,10 +270,8 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
accumulated_intersection: store[cause].get(package).unwrap().negate(),
};
self.next_global_index += 1;
let pa_last_index = self.package_assignments.len().saturating_sub(1);
match self.package_assignments.entry(package) {
Entry::Occupied(mut occupied) => {
let idx = occupied.index();
let pa = occupied.get_mut();
pa.highest_decision_level = self.current_decision_level;
match &mut pa.assignments_intersection {
Expand All @@ -287,10 +283,7 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
*t = t.intersection(&dated_derivation.accumulated_intersection);
dated_derivation.accumulated_intersection = t.clone();
if t.is_positive() {
// we can use `swap_indices` to make `prioritize_decision_level` only go down by 1
// but the copying is slower then the larger search
self.prioritize_decision_level =
std::cmp::min(self.prioritize_decision_level, idx);
self.outdated_priorities.insert(package);
}
}
}
Expand All @@ -299,8 +292,7 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
Entry::Vacant(v) => {
let term = dated_derivation.accumulated_intersection.clone();
if term.is_positive() {
self.prioritize_decision_level =
std::cmp::min(self.prioritize_decision_level, pa_last_index);
self.outdated_priorities.insert(package);
}
v.insert(PackageAssignments {
smallest_decision_level: self.current_decision_level,
Expand All @@ -316,29 +308,27 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
pub(crate) fn pick_highest_priority_pkg(
&mut self,
mut prioritizer: impl FnMut(Id<DP::P>, &DP::VS) -> DP::Priority,
) -> Option<Id<DP::P>> {
let check_all = self.prioritize_decision_level
== self.current_decision_level.0.saturating_sub(1) as usize;
let current_decision_level = self.current_decision_level;
) -> Option<(Id<DP::P>, &DP::VS)> {
let prioritized_potential_packages = &mut self.prioritized_potential_packages;
self.package_assignments
.get_range(self.prioritize_decision_level..)
.unwrap()
.iter()
.filter(|(_, pa)| {
// We only actually need to update the package if it has been changed
// since the last time we called prioritize.
// Which means it's highest decision level is the current decision level,
// or if we backtracked in the meantime.
check_all || pa.highest_decision_level == current_decision_level
})
.filter_map(|(&p, pa)| pa.assignments_intersection.potential_package_filter(p))
.for_each(|(p, r)| {
let priority = prioritizer(p, r);
prioritized_potential_packages.push(p, (priority, Reverse(p.into_raw() as u32)));
});
self.prioritize_decision_level = self.package_assignments.len();
prioritized_potential_packages.pop().map(|(p, _)| p)
while let Some(p) = self.outdated_priorities.pop() {
let Some(pa) = self.package_assignments.get(&p) else {
continue;
};
let Some(r) = pa.assignments_intersection.potential_package_filter() else {
continue;
};
let priority = prioritizer(p, r);
prioritized_potential_packages.push(p, (priority, Reverse(p.into_raw() as u32)));
}
while let Some(p) = self.prioritized_potential_packages.pop().map(|(p, _)| p) {
let Some(pa) = self.package_assignments.get(&p) else {
continue;
};
if let Some(r) = pa.assignments_intersection.potential_package_filter() {
return Some((p, r));
}
}
None
}

/// If a partial solution has, for every positive derivation,
Expand Down Expand Up @@ -378,12 +368,20 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
/// Backtrack the partial solution to a given decision level.
pub(crate) fn backtrack(&mut self, decision_level: DecisionLevel) {
self.current_decision_level = decision_level;
self.package_assignments.retain(|_, pa| {
self.package_assignments.retain(|p, pa| {
if pa.smallest_decision_level > decision_level {
// Remove all entries that have a smallest decision level higher than the backtrack target.
false
} else if pa.highest_decision_level <= decision_level {
// Do not change entries older than the backtrack decision level target.
if pa
.assignments_intersection
.potential_package_filter()
.is_some()
&& self.prioritized_potential_packages.get(p).is_none()
{
self.outdated_priorities.insert(*p);
}
true
} else {
// smallest_decision_level <= decision_level < highest_decision_level
Expand All @@ -408,12 +406,15 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
// Reset the assignments intersection.
pa.assignments_intersection =
AssignmentsIntersection::Derivations(last.accumulated_intersection.clone());

self.prioritized_potential_packages.remove(p);
if pa.assignments_intersection.term().is_positive() {
self.outdated_priorities.insert(*p);
}
true
}
});
// Throw away all stored priority levels, And mark that they all need to be recomputed.
self.prioritized_potential_packages.clear();
self.prioritize_decision_level = self.current_decision_level.0.saturating_sub(1) as usize;
// Throw away all stored priority levels and mark them for recomputing
Copy link
Member

Choose a reason for hiding this comment

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

the code this comment refers to has been removed.

self.has_ever_backtracked = true;
}

Expand Down Expand Up @@ -648,12 +649,12 @@ impl<VS: VersionSet> AssignmentsIntersection<VS> {
/// selected version (no "decision")
/// and if it contains at least one positive derivation term
/// in the partial solution.
fn potential_package_filter<P: Package>(&self, package: Id<P>) -> Option<(Id<P>, &VS)> {
fn potential_package_filter(&self) -> Option<&VS> {
match self {
Self::Decision { .. } => None,
Self::Derivations(term_intersection) => {
if term_intersection.is_positive() {
Some((package, term_intersection.unwrap_positive()))
Some(term_intersection.unwrap_positive())
} else {
None
}
Expand Down
27 changes: 11 additions & 16 deletions src/solver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ use std::fmt::{Debug, Display};
use log::{debug, info};

use crate::internal::{Id, Incompatibility, State};
use crate::{DependencyConstraints, Map, Package, PubGrubError, SelectedDependencies, VersionSet};
use crate::{
DependencyConstraints, Map, Package, PubGrubError, SelectedDependencies, Term, VersionSet,
};

/// Statistics on how often a package conflicted with other packages.
#[derive(Debug, Default, Clone)]
Expand Down Expand Up @@ -148,7 +150,7 @@ pub fn resolve<DP: DependencyProvider>(
state.partial_solution.display(&state.package_store)
);

let Some(highest_priority_pkg) =
let Some((highest_priority_pkg, term_intersection)) =
state.partial_solution.pick_highest_priority_pkg(|p, r| {
dependency_provider.prioritize(
&state.package_store[p],
Expand All @@ -165,17 +167,8 @@ pub fn resolve<DP: DependencyProvider>(
};
next = highest_priority_pkg;

let term_intersection = state
.partial_solution
.term_intersection_for_package(next)
.ok_or_else(|| {
PubGrubError::Failure("a package was chosen but we don't have a term.".into())
})?;
let decision = dependency_provider
.choose_version(
&state.package_store[next],
term_intersection.unwrap_positive(),
)
.choose_version(&state.package_store[next], term_intersection)
.map_err(PubGrubError::ErrorChoosingPackageVersion)?;

info!(
Expand All @@ -186,17 +179,19 @@ pub fn resolve<DP: DependencyProvider>(
// Pick the next compatible version.
let v = match decision {
None => {
let inc = Incompatibility::no_versions(next, term_intersection.clone());
let inc =
Incompatibility::no_versions(next, Term::Positive(term_intersection.clone()));
state.add_incompatibility(inc);
continue;
}
Some(x) => x,
};

if !term_intersection.contains(&v) {
return Err(PubGrubError::Failure(
"choose_package_version picked an incompatible version".into(),
));
panic!(
"`choose_version` picked an incompatible version for package {}, {} is not in {}",
state.package_store[next], v, term_intersection
);
}

let is_new_dependency = added_dependencies
Expand Down