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: visit less clauses during propagation #66

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
3 changes: 1 addition & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,12 @@ bitvec = "1.0.1"
serde = { version = "1.0", features = ["derive"], optional = true }
futures = { version = "0.3", default-features = false, features = ["alloc"] }
event-listener = "5.3"

indexmap = "2"
tokio = { version = "1.37", features = ["rt"], optional = true }
async-std = { version = "1.12", default-features = false, features = ["alloc", "default"], optional = true }

[dev-dependencies]
insta = "1.39.0"
indexmap = "2"
proptest = "1.2"
tracing-test = { version = "0.2.4", features = ["no-env-filter"] }
tokio = { version = "1.35.1", features = ["time", "rt"] }
Expand Down
9 changes: 5 additions & 4 deletions src/conflict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use petgraph::{
Direction,
};

use crate::internal::arena::ArenaId;
use crate::{
internal::id::{ClauseId, InternalSolvableId, SolvableId, StringId, VersionSetId},
runtime::AsyncRuntime,
Expand Down Expand Up @@ -52,11 +53,11 @@ impl Conflict {
let unresolved_node = graph.add_node(ConflictNode::UnresolvedDependency);

for clause_id in &self.clauses {
let clause = &solver.clauses.borrow()[*clause_id].kind;
let clause = &solver.clauses.kinds.borrow()[clause_id.to_usize()];
match clause {
Clause::InstallRoot => (),
Clause::Excluded(solvable, reason) => {
tracing::info!("{solvable:?} is excluded");
tracing::trace!("{solvable:?} is excluded");
let package_node = Self::add_node(&mut graph, &mut nodes, *solvable);
let excluded_node = excluded_nodes
.entry(*reason)
Expand All @@ -76,7 +77,7 @@ impl Conflict {
unreachable!("The version set was used in the solver, so it must have been cached. Therefore cancellation is impossible here and we cannot get an `Err(...)`")
});
if candidates.is_empty() {
tracing::info!(
tracing::trace!(
"{package_id:?} requires {version_set_id:?}, which has no candidates"
);
graph.add_edge(
Expand All @@ -86,7 +87,7 @@ impl Conflict {
);
} else {
for &candidate_id in candidates {
tracing::info!("{package_id:?} requires {candidate_id:?}");
tracing::trace!("{package_id:?} requires {candidate_id:?}");

let candidate_node =
Self::add_node(&mut graph, &mut nodes, candidate_id.into());
Expand Down
6 changes: 3 additions & 3 deletions src/internal/arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ impl<TId: ArenaId, TValue> Arena<TId, TValue> {
pub fn get_two_mut(&mut self, a: TId, b: TId) -> (&mut TValue, &mut TValue) {
let a_index = a.to_usize();
let b_index = b.to_usize();
assert!(a_index < self.len());
assert!(b_index < self.len());
assert_ne!(a_index, b_index);
debug_assert!(a_index < self.len());
debug_assert!(b_index < self.len());
debug_assert_ne!(a_index, b_index);
let (a_chunk, a_offset) = Self::chunk_and_offset(a_index);
let (b_chunk, b_offset) = Self::chunk_and_offset(b_index);
// SAFE: because we check that the indices are less than the length and that both indices do
Expand Down
17 changes: 3 additions & 14 deletions src/internal/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,10 @@ impl ArenaId for VersionSetUnionId {
#[cfg_attr(feature = "serde", serde(transparent))]
pub struct SolvableId(pub u32);

/// Internally used id for solvables that can also represent root and null.
/// Internally used id for solvables that can also represent the root.
#[repr(transparent)]
#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug, Ord, PartialOrd)]
pub(crate) struct InternalSolvableId(u32);

const INTERNAL_SOLVABLE_NULL: u32 = u32::MAX;
pub(crate) struct InternalSolvableId(pub u32);
const INTERNAL_SOLVABLE_ROOT: u32 = 0;

impl InternalSolvableId {
Expand All @@ -97,17 +95,9 @@ impl InternalSolvableId {
self.0 == 0
}

pub const fn null() -> Self {
Self(u32::MAX)
}

pub const fn is_null(self) -> bool {
self.0 == u32::MAX
}

pub const fn as_solvable(self) -> Option<SolvableId> {
match self.0 {
INTERNAL_SOLVABLE_NULL | INTERNAL_SOLVABLE_ROOT => None,
INTERNAL_SOLVABLE_ROOT => None,
x => Some(SolvableId(x - 1)),
}
}
Expand All @@ -126,7 +116,6 @@ impl<'i, I: Interner> Display for DisplayInternalSolvable<'i, I> {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match self.id.0 {
INTERNAL_SOLVABLE_ROOT => write!(f, "<root>"),
INTERNAL_SOLVABLE_NULL => write!(f, "<null>"),
x => {
write!(f, "{}", self.interner.display_solvable(SolvableId(x - 1)))
}
Expand Down
5 changes: 3 additions & 2 deletions src/internal/mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl<TId: ArenaId, TValue> Mapping<TId, TValue> {
}

/// Insert into the mapping with the specific value
pub fn insert(&mut self, id: TId, value: TValue) {
pub fn insert(&mut self, id: TId, value: TValue) -> Option<TValue> {
let idx = id.to_usize();
let (chunk, offset) = Self::chunk_and_offset(idx);

Expand All @@ -59,9 +59,10 @@ impl<TId: ArenaId, TValue> Mapping<TId, TValue> {
self.chunks
.resize_with(chunk + 1, || std::array::from_fn(|_| None));
}
self.chunks[chunk][offset] = Some(value);
let previous_value = self.chunks[chunk][offset].replace(value);
self.len += 1;
self.max = self.max.max(idx);
previous_value
}

/// Get a specific value in the mapping with bound checks
Expand Down
Loading