Skip to content

Commit

Permalink
fix: use nohash in the resolver for the activation keys
Browse files Browse the repository at this point in the history
  • Loading branch information
x-hgg-x committed Oct 10, 2024
1 parent de3e0b8 commit ee74105
Show file tree
Hide file tree
Showing 8 changed files with 108 additions and 27 deletions.
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ libgit2-sys = "0.17.0"
libloading = "0.8.5"
memchr = "2.7.4"
miow = "0.6.0"
nohash-hasher = "0.2.0"
opener = "0.7.1"
openssl = "=0.10.57" # See rust-lang/cargo#13546 and openssl/openssl#23376 for pinning
openssl-sys = "=0.9.92" # See rust-lang/cargo#13546 and openssl/openssl#23376 for pinning
Expand Down Expand Up @@ -182,6 +183,7 @@ jobserver.workspace = true
lazycell.workspace = true
libgit2-sys.workspace = true
memchr.workspace = true
nohash-hasher.workspace = true
opener.workspace = true
os_info.workspace = true
pasetors.workspace = true
Expand Down
2 changes: 1 addition & 1 deletion crates/resolver-tests/src/sat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ impl SatResolver {
&mut solver,
var_for_is_packages_used
.iter()
.map(|(p, &v)| (p.as_activations_key(), v)),
.map(|(p, &v)| (p.activation_key(), v)),
);

for pkg in by_name.values().flatten() {
Expand Down
60 changes: 52 additions & 8 deletions src/cargo/core/activation_key.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,61 @@
use std::collections::HashSet;
use std::hash::{Hash, Hasher};
use std::num::NonZeroU64;
use std::sync::{Mutex, OnceLock};

use crate::core::SourceId;
use crate::util::interning::InternedString;

/// Find the activated version of a crate based on the name, source, and semver compatibility.
/// By storing this in a hash map we ensure that there is only one
/// semver compatible version of each crate.
/// This all so stores the `ContextAge`.
pub type ActivationsKey = (InternedString, SourceId, SemverCompatibility);
static ACTIVATION_KEY_CACHE: OnceLock<Mutex<HashSet<&'static ActivationKeyInner>>> =
OnceLock::new();

/// A type that represents when cargo treats two Versions as compatible.
/// Versions `a` and `b` are compatible if their left-most nonzero digit is the
/// same.
type ActivationKeyInner = (InternedString, SourceId, SemverCompatibility);

/// The activated version of a crate is based on the name, source, and semver compatibility.
#[derive(Clone, Copy, Eq)]
pub struct ActivationKey {
inner: &'static ActivationKeyInner,
}

impl From<ActivationKeyInner> for ActivationKey {
fn from(inner: ActivationKeyInner) -> Self {
let mut cache = ACTIVATION_KEY_CACHE
.get_or_init(|| Default::default())
.lock()
.unwrap();
let inner = cache.get(&inner).cloned().unwrap_or_else(|| {
let inner = Box::leak(Box::new(inner));
cache.insert(inner);
inner
});
Self { inner }
}
}

impl ActivationKey {
/// This function is used for the `Eq` and `Hash` impls to implement a "no hash" hashable value.
/// This is possible since all `ActivationKey` are already interned in a `HashSet`.
fn key(&self) -> u64 {
std::ptr::from_ref(self.inner) as u64
}
}

impl PartialEq for ActivationKey {
fn eq(&self, other: &Self) -> bool {
self.key() == other.key()
}
}

impl nohash_hasher::IsEnabled for ActivationKey {}

impl Hash for ActivationKey {
fn hash<H: Hasher>(&self, state: &mut H) {
state.write_u64(self.key());
}
}

/// A type that represents when cargo treats two versions as compatible.
/// Versions `a` and `b` are compatible if their left-most nonzero digit is the same.
#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug, PartialOrd, Ord)]
pub enum SemverCompatibility {
Major(NonZeroU64),
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pub use self::activation_key::ActivationsKey;
pub use self::activation_key::ActivationKey;
pub use self::dependency::{Dependency, SerializedDependency};
pub use self::features::{CliUnstable, Edition, Feature, Features};
pub use self::manifest::{EitherManifest, VirtualManifest};
Expand Down
28 changes: 24 additions & 4 deletions src/cargo/core/package_id.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::cmp::Ordering;
use std::collections::HashSet;
use std::fmt::{self, Formatter};
use std::hash;
Expand All @@ -10,7 +11,7 @@ use std::sync::OnceLock;
use serde::de;
use serde::ser;

use crate::core::ActivationsKey;
use crate::core::ActivationKey;
use crate::core::PackageIdSpec;
use crate::core::SourceId;
use crate::util::interning::InternedString;
Expand All @@ -24,13 +25,31 @@ pub struct PackageId {
inner: &'static PackageIdInner,
}

#[derive(PartialOrd, Eq, Ord)]
struct PackageIdInner {
name: InternedString,
version: semver::Version,
source_id: SourceId,
// This field is used as a cache to improve the resolver speed,
// and is not included in the `Eq`, `Hash` and `Ord` impls.
activation_key: ActivationKey,
}

impl Ord for PackageIdInner {
fn cmp(&self, other: &Self) -> Ordering {
let self_key = (self.name, &self.version, self.source_id);
let other_key = (other.name, &other.version, other.source_id);
self_key.cmp(&other_key)
}
}

impl PartialOrd for PackageIdInner {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(&other))
}
}

impl Eq for PackageIdInner {}

// Custom equality that uses full equality of SourceId, rather than its custom equality.
//
// The `build` part of the version is usually ignored (like a "comment").
Expand Down Expand Up @@ -136,6 +155,7 @@ impl PackageId {

pub fn new(name: InternedString, version: semver::Version, source_id: SourceId) -> PackageId {
let inner = PackageIdInner {
activation_key: (name, source_id, (&version).into()).into(),
name,
version,
source_id,
Expand All @@ -161,8 +181,8 @@ impl PackageId {
pub fn source_id(self) -> SourceId {
self.inner.source_id
}
pub fn as_activations_key(self) -> ActivationsKey {
(self.name(), self.source_id(), self.version().into())
pub fn activation_key(self) -> ActivationKey {
self.inner.activation_key
}

pub fn with_source_id(self, source: SourceId) -> PackageId {
Expand Down
29 changes: 19 additions & 10 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use super::dep_cache::RegistryQueryer;
use super::errors::ActivateResult;
use super::types::{ConflictMap, ConflictReason, FeaturesSet, ResolveOpts};
use super::RequestedFeatures;
use crate::core::{ActivationsKey, Dependency, PackageId, Summary};
use crate::core::{ActivationKey, Dependency, PackageId, Summary};
use crate::util::interning::InternedString;
use crate::util::Graph;
use anyhow::format_err;
Expand All @@ -26,8 +26,13 @@ pub struct ResolverContext {
pub parents: Graph<PackageId, im_rc::HashSet<Dependency, rustc_hash::FxBuildHasher>>,
}

pub type Activations =
im_rc::HashMap<ActivationsKey, (Summary, ContextAge), rustc_hash::FxBuildHasher>;
/// By storing activation keys in a `HashMap` we ensure that there is only one
/// semver compatible version of each crate.
type Activations = im_rc::HashMap<
ActivationKey,
(Summary, ContextAge),
nohash_hasher::BuildNoHashHasher<ActivationKey>,
>;

/// When backtracking it can be useful to know how far back to go.
/// The `ContextAge` of a `Context` is a monotonically increasing counter of the number
Expand Down Expand Up @@ -62,7 +67,7 @@ impl ResolverContext {
) -> ActivateResult<bool> {
let id = summary.package_id();
let age: ContextAge = self.age;
match self.activations.entry(id.as_activations_key()) {
match self.activations.entry(id.activation_key()) {
im_rc::hashmap::Entry::Occupied(o) => {
debug_assert_eq!(
&o.get().0,
Expand Down Expand Up @@ -101,7 +106,7 @@ impl ResolverContext {
// versions came from a `[patch]` source.
if let Some((_, dep)) = parent {
if dep.source_id() != id.source_id() {
let key = (id.name(), dep.source_id(), id.version().into());
let key = (id.name(), dep.source_id(), id.version().into()).into();
let prev = self.activations.insert(key, (summary.clone(), age));
if let Some((previous_summary, _)) = prev {
return Err(
Expand Down Expand Up @@ -145,9 +150,13 @@ impl ResolverContext {

/// If the package is active returns the `ContextAge` when it was added
pub fn is_active(&self, id: PackageId) -> Option<ContextAge> {
self.activations
.get(&id.as_activations_key())
.and_then(|(s, l)| if s.package_id() == id { Some(*l) } else { None })
let (summary, age) = self.activations.get(&id.activation_key())?;

if summary.package_id() == id {
Some(*age)
} else {
None
}
}

/// Checks whether all of `parent` and the keys of `conflicting activations`
Expand All @@ -163,8 +172,8 @@ impl ResolverContext {
max = std::cmp::max(max, self.is_active(parent)?);
}

for id in conflicting_activations.keys() {
max = std::cmp::max(max, self.is_active(*id)?);
for &id in conflicting_activations.keys() {
max = std::cmp::max(max, self.is_active(id)?);
}
Some(max)
}
Expand Down
5 changes: 2 additions & 3 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,7 @@ fn activate_deps_loop(
let mut backtrack_stack = Vec::new();
let mut remaining_deps = RemainingDeps::new();

// `past_conflicting_activations` is a cache of the reasons for each time we
// backtrack.
// `past_conflicting_activations` is a cache of the reasons for each time we backtrack.
let mut past_conflicting_activations = conflict_cache::ConflictCache::new();

// Activate all the initial summaries to kick off some work.
Expand Down Expand Up @@ -775,7 +774,7 @@ impl RemainingCandidates {
//
// Here we throw out our candidate if it's *compatible*, yet not
// equal, to all previously activated versions.
if let Some((a, _)) = cx.activations.get(&b_id.as_activations_key()) {
if let Some((a, _)) = cx.activations.get(&b_id.activation_key()) {
if *a != b {
conflicting_prev_active
.entry(a.package_id())
Expand Down

0 comments on commit ee74105

Please sign in to comment.