From 8cd80b3e64deb5c3fa6af40fe317adac3937e91e Mon Sep 17 00:00:00 2001 From: Bas Zalmstra <4995967+baszalmstra@users.noreply.github.com> Date: Sun, 23 Aug 2026 22:56:22 +0200 Subject: [PATCH] perf: use native words in IndexedSet and hoist forbid-name lookup Store IndexedSet bits in native machine words and resolve a candidate list's name once when registering forbid targets. Add the candidate-list name invariant in debug builds and keep IndexedSet's coverage next to its implementation. --- Cargo.lock | 40 ---------- Cargo.toml | 1 - src/solver/encoding.rs | 29 +++++--- src/utils/indexed_set.rs | 96 +++++++++++++++++++++--- tests/solver/main.rs | 156 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 261 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 83f23bb4..6f933c7c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -232,18 +232,6 @@ version = "2.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b588b76d00fde79687d7646a9b5bdf3cc0f655e0bbd080335a95d7e96f3587da" -[[package]] -name = "bitvec" -version = "1.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ddcec3d12c579d40898fe0a9a358a803c23e9c52ca3c425707f81c9436211837" -dependencies = [ - "funty", - "radium", - "tap", - "wyz", -] - [[package]] name = "blocking" version = "1.6.2" @@ -492,12 +480,6 @@ version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d9c4f5dac5e15c24eb999c26181a6ca40b39fe946cbe4c263c7209467bc83af2" -[[package]] -name = "funty" -version = "2.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6d5a32815ae3f33302d95fdcb2ce17862f8c65363dcfd29360480ba1001fc9c" - [[package]] name = "futures" version = "0.3.34" @@ -899,12 +881,6 @@ version = "5.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" -[[package]] -name = "radium" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dc33ff2d4973d518d823d61aa239014831e521c75da58e3df4840d3f47749d09" - [[package]] name = "rand" version = "0.9.3" @@ -983,7 +959,6 @@ version = "0.12.0" dependencies = [ "ahash", "async-std", - "bitvec", "chumsky", "elsa", "event-listener 5.4.2", @@ -1206,12 +1181,6 @@ dependencies = [ "unicode-width", ] -[[package]] -name = "tap" -version = "1.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "55937e1799185b12863d447f42597ed69d9928686b8d88a1df17376a097d8369" - [[package]] name = "tempfile" version = "3.20.0" @@ -1701,15 +1670,6 @@ dependencies = [ "bitflags", ] -[[package]] -name = "wyz" -version = "0.5.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "05f360fc0b24296329c78fda852a1e9ae82de9cf7b27dae4b7f62f118f77b9ed" -dependencies = [ - "tap", -] - [[package]] name = "zerocopy" version = "0.8.26" diff --git a/Cargo.toml b/Cargo.toml index b52027bb..ade00d57 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,7 +41,6 @@ itertools = "0.15" petgraph = "0.8" tracing = "0.1.41" elsa = "1.11.2" -bitvec = "1.0.1" serde = { version = "1.0", features = ["derive"], optional = true } futures = { version = "0.3", default-features = false, features = ["alloc", "async-await"] } event-listener = "5.4" diff --git a/src/solver/encoding.rs b/src/solver/encoding.rs index 2c774db6..41563eaa 100644 --- a/src/solver/encoding.rs +++ b/src/solver/encoding.rs @@ -390,15 +390,26 @@ impl<'a, 'cache, D: DependencyProvider> Encoder<'a, 'cache, D> { // // We only add these clauses for packages that can actually be selected to // reduce the overall number of clauses. - for (solvable, variable_id) in candidates - .iter() - .zip(version_set_variables.iter()) - .flat_map(|(&candidates, variable)| { - candidates.iter().copied().zip(variable.iter().copied()) - }) - { - let name_id = self.cache.provider().solvable_name(solvable); - self.register_forbid_target(name_id, variable_id); + for (&candidates, variables) in candidates.iter().zip(version_set_variables.iter()) { + let Some(&first_solvable) = candidates.first() else { + continue; + }; + let name_id = self.cache.provider().solvable_name(first_solvable); + debug_assert!( + candidates + .iter() + .all(|&solvable| self.cache.provider().solvable_name(solvable) == name_id), + "all candidates in a version set must have the same package name" + ); + if self.state.allow_multiple_names.contains(name_id) { + continue; + } + let pending = self.pending_forbid_clauses.entry(name_id).or_default(); + for &variable_id in variables { + if self.forbid_seen.insert(variable_id) { + pending.push(variable_id); + } + } } // Queue requesting the dependencies of the candidates as well if they are diff --git a/src/utils/indexed_set.rs b/src/utils/indexed_set.rs index 2ad1e964..90cd89ce 100644 --- a/src/utils/indexed_set.rs +++ b/src/utils/indexed_set.rs @@ -1,21 +1,21 @@ use std::marker::PhantomData; -use bitvec::vec::BitVec; - use crate::id::DenseIndex; +const WORD_BITS: usize = usize::BITS as usize; + /// A dense set keyed by a [`DenseIndex`]. Equivalent to a `HashSet` but -/// backed by a [`BitVec`], so test-and-set is O(1) with no hashing overhead. -/// Grows on demand to fit the largest inserted index. +/// backed by native machine words, so test-and-set is O(1) with no hashing +/// overhead. Grows on demand to fit the largest inserted index. pub struct IndexedSet { - bits: BitVec, + words: Vec, _marker: PhantomData Id>, } impl Default for IndexedSet { fn default() -> Self { Self { - bits: BitVec::new(), + words: Vec::new(), _marker: PhantomData, } } @@ -26,16 +26,90 @@ impl IndexedSet { #[inline] pub fn insert(&mut self, id: Id) -> bool { let idx = id.to_index(); - if idx >= self.bits.len() { - self.bits.resize(idx + 1, false); + let (word, bit) = (idx / WORD_BITS, 1usize << (idx % WORD_BITS)); + if word >= self.words.len() { + self.words.resize(word + 1, 0); } - // SAFETY: `resize` above guarantees `idx < self.bits.len()`. - !unsafe { self.bits.replace_unchecked(idx, true) } + let entry = &mut self.words[word]; + let was_set = *entry & bit != 0; + *entry |= bit; + !was_set } /// Returns `true` if `id` is present. #[inline] pub fn contains(&self, id: Id) -> bool { - self.bits.get(id.to_index()).is_some_and(|b| *b) + let idx = id.to_index(); + self.words + .get(idx / WORD_BITS) + .is_some_and(|word| word & (1usize << (idx % WORD_BITS)) != 0) + } +} + +#[cfg(test)] +mod tests { + use super::{IndexedSet, WORD_BITS}; + use crate::{DenseIndex, NameId}; + + fn id(index: usize) -> NameId { + NameId::from_index(index) + } + + #[test] + fn word_boundaries() { + let mut set = IndexedSet::::default(); + for index in [ + 0, + WORD_BITS - 1, + WORD_BITS, + WORD_BITS + 1, + WORD_BITS * 2 - 1, + WORD_BITS * 2, + ] { + assert!(set.insert(id(index))); + assert!(set.contains(id(index))); + assert!(!set.insert(id(index))); + } + for index in [1, WORD_BITS - 2, WORD_BITS + 2, WORD_BITS * 2 + 1] { + assert!(!set.contains(id(index))); + } + } + + #[test] + fn sparse_and_large_indices() { + let mut set = IndexedSet::::default(); + assert!(!set.contains(id(10_000))); + assert!(set.insert(id(10_000))); + assert!(set.contains(id(10_000))); + assert!(!set.contains(id(9_999))); + assert!(!set.contains(id(10_001))); + + assert!(set.insert(id(0))); + assert!(set.contains(id(0))); + assert!(set.contains(id(10_000))); + assert!(!set.contains(id(WORD_BITS - 1))); + assert!(!set.contains(id(WORD_BITS))); + assert!(!set.contains(id(WORD_BITS + 1))); + } + + #[test] + fn duplicate_inserts_and_sequential_fill() { + let mut set = IndexedSet::::default(); + for index in 0..=WORD_BITS * 32 { + assert!(set.insert(id(index))); + } + for index in 0..=WORD_BITS * 32 { + assert!(!set.insert(id(index))); + assert!(set.contains(id(index))); + } + assert!(!set.contains(id(WORD_BITS * 32 + 1))); + + let mut reverse = IndexedSet::::default(); + for index in (0..=WORD_BITS * 4).rev() { + assert!(reverse.insert(id(index))); + } + for index in 0..=WORD_BITS * 4 { + assert!(reverse.contains(id(index))); + } } } diff --git a/tests/solver/main.rs b/tests/solver/main.rs index 1015d079..6d26355b 100644 --- a/tests/solver/main.rs +++ b/tests/solver/main.rs @@ -2541,3 +2541,159 @@ mod allow_multiple_versions { "); } } + +mod forbid_registration_regression { + use super::*; + + /// Solves a simple provider built from `packages` for the given root specs + /// and returns the sorted transaction string. + fn solve_to_string(packages: &[(&str, u32, Vec<&str>)], specs: &[&str]) -> String { + let mut provider = BundleBoxProvider::from_packages(packages); + let requirements = provider.requirements(specs); + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + transaction_to_string(solver.provider(), &solved) + } + + /// A union over two non-overlapping version sets of the same package must + /// install exactly one version. Both lists must land in the same + /// `pending_forbid_clauses` bucket so the forbid-multiple clause is built. + #[test] + fn test_union_same_package_single_version() { + let result = solve_to_string( + &[("x", 1, vec![]), ("x", 2, vec![]), ("x", 3, vec![])], + &["x 1..2 | x 3..4"], + ); + // Exactly one version of `x` must be selected: the forbid-multiple + // clause has to be registered for both candidate lists. + assert_eq!(result.matches("x=").count(), 1, "{result}"); + assert_snapshot!(result, @"x=1"); + } + + /// Overlapping same-name version sets share candidate variables. The global + /// `forbid_seen` dedup must keep the duplicate from being pushed twice + /// while still registering every distinct candidate. + #[test] + fn test_union_overlapping_same_package_dedup() { + let result = solve_to_string( + &[("x", 1, vec![]), ("x", 2, vec![]), ("x", 3, vec![])], + &["x 1..3 | x 2..4"], + ); + assert_eq!(result.matches("x=").count(), 1, "{result}"); + assert_snapshot!(result, @"x=2"); + } + + /// An empty candidate list in the *first* version set of a union must be + /// skipped without disturbing the following non-empty list. + #[test] + fn test_union_empty_first_candidate_list() { + let result = solve_to_string(&[("a", 1, vec![]), ("b", 1, vec![])], &["a 5..6 | b 1..2"]); + assert_snapshot!(result, @"b=1"); + } + + /// An empty candidate list in a *later* version set of a union must also be + /// skipped while the earlier list still registers its forbid target. + #[test] + fn test_union_empty_second_candidate_list() { + let result = solve_to_string(&[("x", 1, vec![]), ("x", 2, vec![])], &["x 1..2 | x 5..6"]); + assert_snapshot!(result, @"x=1"); + } + + /// A locked solvable combined with a union requirement: the lock forbids the + /// non-locked candidates, so the union branch containing them resolves to the + /// locked version. + #[test] + fn test_locked_through_union() { + let mut provider = BundleBoxProvider::from_packages(&[ + ("x", 1, vec![]), + ("x", 2, vec![]), + ("x", 3, vec![]), + ]); + provider.set_locked("x", 1); + + let requirements = provider.requirements(&["x 2..3 | x 1..2"]); + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + assert_snapshot!(result, @"x=1"); + } + + /// An excluded solvable inside a union branch: the exclusion clause forces + /// that branch off and the other union branch is selected. + #[test] + fn test_excluded_through_union() { + let mut provider = BundleBoxProvider::from_packages(&[ + ("x", 1, vec![]), + ("x", 2, vec![]), + ("x", 3, vec![]), + ]); + provider.exclude("x", 2, "it is externally excluded"); + + let requirements = provider.requirements(&["x 2..3 | x 1..2"]); + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + assert_snapshot!(result, @"x=1"); + } + + /// A union whose members are an `allow_multiple` package and a *normal* + /// package: skipping forbid registration for the multiversion member must + /// not leak into the sibling member. `single` is forced to version 2 while + /// the union offers `single=1`; without the normal package's forbid clause + /// both `single=1` and `single=2` would be installable. + #[test] + fn test_allow_multiple_does_not_disable_forbid_for_sibling_union_member() { + let mut provider = BundleBoxProvider::from_packages(&[ + ("multi", 0, vec![]), + ("single", 1, vec![]), + ("single", 2, vec![]), + ("app-a", 1, vec!["multi 0..1 | single 1..2"]), + ("app-b", 1, vec!["single 2..3"]), + ]); + provider.set_allow_multiple("multi"); + + let requirements = provider.requirements(&["app-a", "app-b"]); + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + assert_snapshot!(result, @r" + app-a=1 + app-b=1 + multi=0 + single=2 + "); + } + + /// `allow_multiple` combined with an empty candidate list and a union, all + /// reached transitively: `helper 2..3` has no candidates (empty list is + /// skipped) while the two non-overlapping `multi` ranges install two + /// versions because forbid clauses are (correctly) suppressed for it. + #[test] + fn test_allow_multiple_union_with_empty_branch_transitive() { + let mut provider = BundleBoxProvider::from_packages(&[ + ("multi", 1, vec![]), + ("multi", 2, vec![]), + ("multi", 3, vec![]), + ("helper", 1, vec![]), + ("app-a", 1, vec!["multi 1..2 | helper 2..3"]), + ("app-b", 1, vec!["multi 3..4"]), + ]); + provider.set_allow_multiple("multi"); + + let requirements = provider.requirements(&["app-a", "app-b"]); + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + assert_snapshot!(result, @r" + app-a=1 + app-b=1 + multi=1 + multi=3 + "); + } +}