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.

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

perf: small memory performance optimizations #35

Merged
merged 6 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ readme = "README.md"
resolver = "2"

[dependencies]
ahash = "0.8.11"
itertools = "0.13"
petgraph = "0.6.5"
tracing = "0.1.40"
Expand Down
4 changes: 2 additions & 2 deletions src/internal/arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,15 @@ impl<TId: ArenaId, TValue> Arena<TId, TValue> {
}

/// Returns an iterator over the elements of the arena.
pub fn iter(&self) -> ArenaIter<TId, TValue> {
pub fn iter(&self) -> ArenaIter<'_, TId, TValue> {
ArenaIter {
arena: self,
index: 0,
}
}

/// Returns an mutable iterator over the elements of the arena.
pub fn iter_mut(&mut self) -> ArenaIterMut<TId, TValue> {
pub fn iter_mut(&mut self) -> ArenaIterMut<'_, TId, TValue> {
ArenaIterMut {
arena: self,
index: 0,
Expand Down
3 changes: 1 addition & 2 deletions src/internal/frozen_copy_map.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
use std::borrow::Borrow;
use std::cell::UnsafeCell;
use std::collections::hash_map::RandomState;
use std::collections::HashMap;
use std::hash::{BuildHasher, Hash};

/// An insert only map where items can only be returned by cloning the values. This ensures that the
/// map can safely be used in an immutable context.
pub struct FrozenCopyMap<K, V, S = RandomState> {
pub struct FrozenCopyMap<K, V, S = ahash::RandomState> {
map: UnsafeCell<HashMap<K, V, S>>,
}

Expand Down
2 changes: 1 addition & 1 deletion src/internal/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ impl SolvableId {
pub fn display<VS: VersionSet, N: PackageName + Display>(
self,
pool: &Pool<VS, N>,
) -> DisplaySolvable<VS, N> {
) -> DisplaySolvable<'_, VS, N> {
pool.resolve_internal_solvable(self).display(pool)
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/internal/mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ impl<TId: ArenaId, TValue> Mapping<TId, TValue> {
}

/// Returns an iterator over all the existing key value pairs.
pub fn iter(&self) -> MappingIter<TId, TValue> {
pub fn iter(&self) -> MappingIter<'_, TId, TValue> {
MappingIter {
mapping: self,
offset: 0,
Expand Down
6 changes: 3 additions & 3 deletions src/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,19 @@ pub struct Pool<VS: VersionSet, N: PackageName = String> {
package_names: Arena<NameId, N>,

/// Map from package names to the id of their interned counterpart
pub(crate) names_to_ids: FrozenCopyMap<N, NameId>,
pub(crate) names_to_ids: FrozenCopyMap<N, NameId, ahash::RandomState>,

/// Interned strings
strings: Arena<StringId, String>,

/// Map from package names to the id of their interned counterpart
pub(crate) string_to_ids: FrozenCopyMap<String, StringId>,
pub(crate) string_to_ids: FrozenCopyMap<String, StringId, ahash::RandomState>,

/// Interned match specs
pub(crate) version_sets: Arena<VersionSetId, (NameId, VS)>,

/// Map from version set to the id of their interned counterpart
version_set_to_id: FrozenCopyMap<(NameId, VS), VersionSetId>,
version_set_to_id: FrozenCopyMap<(NameId, VS), VersionSetId, ahash::RandomState>,
}

impl<VS: VersionSet, N: PackageName> Default for Pool<VS, N> {
Expand Down
9 changes: 5 additions & 4 deletions src/problem.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Types to examine why a problem was unsatisfiable, and to report the causes to the user.

use std::collections::{HashMap, HashSet};
use ahash::HashMap;
use std::collections::HashSet;
use std::fmt;
use std::fmt::{Display, Formatter};
use std::hash::Hash;
Expand Down Expand Up @@ -277,7 +278,7 @@ impl ProblemGraph {
let merged_nodes = if simplify {
self.simplify(pool)
} else {
HashMap::new()
HashMap::default()
};

write!(f, "digraph {{")?;
Expand Down Expand Up @@ -357,7 +358,7 @@ impl ProblemGraph {
let graph = &self.graph;

// Gather information about nodes that can be merged
let mut maybe_merge = HashMap::new();
let mut maybe_merge = HashMap::default();
for node_id in graph.node_indices() {
let candidate = match graph[node_id] {
ProblemNode::UnresolvedDependency | ProblemNode::Excluded(_) => continue,
Expand Down Expand Up @@ -653,7 +654,7 @@ impl<'pool, VS: VersionSet, N: PackageName + Display, M: SolvableDisplay<VS, N>>
fn fmt_graph(
&self,
f: &mut Formatter<'_>,
top_level_edges: &[EdgeReference<ProblemEdge>],
top_level_edges: &[EdgeReference<'_, ProblemEdge>],
top_level_indent: bool,
) -> fmt::Result
where
Expand Down
10 changes: 6 additions & 4 deletions src/solver/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ use crate::{
Candidates, Dependencies, DependencyProvider, NameId, PackageName, Pool, SolvableId,
VersionSet, VersionSetId,
};
use ahash::HashMap;
use bitvec::vec::BitVec;
use elsa::FrozenMap;
use event_listener::Event;
use std::{any::Any, cell::RefCell, collections::HashMap, marker::PhantomData, rc::Rc};
use std::{any::Any, cell::RefCell, marker::PhantomData, rc::Rc};

/// Keeps a cache of previously computed and/or requested information about solvables and version
/// sets.
Expand All @@ -24,14 +25,15 @@ pub struct SolverCache<VS: VersionSet, N: PackageName, D: DependencyProvider<VS,
package_name_to_candidates_in_flight: RefCell<HashMap<NameId, Rc<Event>>>,

/// A mapping of `VersionSetId` to the candidates that match that set.
version_set_candidates: FrozenMap<VersionSetId, Vec<SolvableId>>,
version_set_candidates: FrozenMap<VersionSetId, Vec<SolvableId>, ahash::RandomState>,

/// A mapping of `VersionSetId` to the candidates that do not match that set (only candidates
/// of the package indicated by the version set are included).
version_set_inverse_candidates: FrozenMap<VersionSetId, Vec<SolvableId>>,
version_set_inverse_candidates: FrozenMap<VersionSetId, Vec<SolvableId>, ahash::RandomState>,

/// A mapping of `VersionSetId` to a sorted list of candidates that match that set.
pub(crate) version_set_to_sorted_candidates: FrozenMap<VersionSetId, Vec<SolvableId>>,
pub(crate) version_set_to_sorted_candidates:
FrozenMap<VersionSetId, Vec<SolvableId>, ahash::RandomState>,

/// A mapping from a solvable to a list of dependencies
solvable_dependencies: Arena<DependenciesId, Dependencies>,
Expand Down
12 changes: 10 additions & 2 deletions src/solver/clause.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,11 @@ impl Clause {
pub fn visit_literals(
&self,
learnt_clauses: &Arena<LearntClauseId, Vec<Literal>>,
version_set_to_sorted_candidates: &FrozenMap<VersionSetId, Vec<SolvableId>>,
version_set_to_sorted_candidates: &FrozenMap<
VersionSetId,
Vec<SolvableId>,
ahash::RandomState,
>,
mut visit: impl FnMut(Literal),
) {
match *self {
Expand Down Expand Up @@ -487,7 +491,11 @@ impl ClauseState {
pub fn next_unwatched_variable(
&self,
learnt_clauses: &Arena<LearntClauseId, Vec<Literal>>,
version_set_to_sorted_candidates: &FrozenMap<VersionSetId, Vec<SolvableId>>,
version_set_to_sorted_candidates: &FrozenMap<
VersionSetId,
Vec<SolvableId>,
ahash::RandomState,
>,
decision_map: &DecisionMap,
) -> Option<SolvableId> {
// The next unwatched variable (if available), is a variable that is:
Expand Down
7 changes: 4 additions & 3 deletions src/solver/decision_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::fmt::{Display, Formatter};
/// < 0: level of decision when the solvable is set to false
#[repr(transparent)]
#[derive(Copy, Clone)]
struct DecisionAndLevel(i64);
struct DecisionAndLevel(i32);

impl DecisionAndLevel {
fn undecided() -> DecisionAndLevel {
Expand All @@ -26,11 +26,12 @@ impl DecisionAndLevel {
}

fn level(self) -> u32 {
self.0.unsigned_abs() as u32
self.0.unsigned_abs()
}

fn with_value_and_level(value: bool, level: u32) -> Self {
Self(if value { level as i64 } else { -(level as i64) })
debug_assert!(level <= (i32::MAX as u32), "level is too large");
Self(if value { level as i32 } else { -(level as i32) })
}
}

Expand Down
7 changes: 4 additions & 3 deletions src/solver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1019,9 +1019,10 @@ impl<VS: VersionSet, N: PackageName + Display, D: DependencyProvider<VS, N>, RT:
let mut predecessor_clause_id: Option<ClauseId> = None;
let mut clause_id = self.watches.first_clause_watching_solvable(pkg);
while !clause_id.is_null() {
if predecessor_clause_id == Some(clause_id) {
panic!("Linked list is circular!");
}
debug_assert!(
predecessor_clause_id != Some(clause_id),
"Linked list is circular!"
);

// Get mutable access to both clauses.
let mut clauses = self.clauses.borrow_mut();
Expand Down
3 changes: 2 additions & 1 deletion tests/solver.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{
any::Any,
cell::{Cell, RefCell},
collections::{HashMap, HashSet},
collections::HashSet,
fmt::{Debug, Display, Formatter},
io::{stderr, Write},
num::ParseIntError,
Expand All @@ -14,6 +14,7 @@ use std::{
time::Duration,
};

use ahash::HashMap;
use indexmap::IndexMap;
use itertools::Itertools;
use resolvo::{
Expand Down