fix: use nohash in the resolver for the activation keys

Author: x-hgg-x

Cargo.lock

@@ -66,6 +66,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
@@ -179,6 +180,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

Cargo.toml

@@ -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().no_hash(), v)),
         );
 
         for pkg in by_name.values().flatten() {

crates/resolver-tests/src/sat.rs

@@ -1,3 +1,4 @@
+use std::cmp::Ordering;
 use std::collections::HashSet;
 use std::fmt::{self, Formatter};
 use std::hash;
@@ -10,6 +11,7 @@ use std::sync::OnceLock;
 use serde::de;
 use serde::ser;
 
+use crate::core::resolver::activation_key::ActivationKey;
 use crate::core::PackageIdSpec;
 use crate::core::SourceId;
 use crate::util::interning::InternedString;
@@ -23,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").
@@ -135,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,
@@ -160,6 +181,9 @@ impl PackageId {
     pub fn source_id(self) -> SourceId {
         self.inner.source_id
     }
+    pub fn activation_key(self) -> ActivationKey {
+        self.inner.activation_key
+    }
 
     pub fn with_source_id(self, source: SourceId) -> PackageId {
         PackageId::new(self.inner.name, self.inner.version.clone(), source)

src/cargo/core/package_id.rs

@@ -0,0 +1,73 @@
+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;
+
+static ACTIVATION_KEY_CACHE: OnceLock<Mutex<HashSet<&'static ActivationKeyInner>>> =
+    OnceLock::new();
+
+type ActivationKeyInner = (InternedString, SourceId, SemverCompatibility);
+
+/// The activated version of a crate is based on the name, source, and semver compatibility.
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+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 }
+    }
+}
+
+/// 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),
+    Minor(NonZeroU64),
+    Patch(u64),
+}
+
+impl From<&semver::Version> for SemverCompatibility {
+    fn from(ver: &semver::Version) -> Self {
+        if let Some(m) = NonZeroU64::new(ver.major) {
+            return SemverCompatibility::Major(m);
+        }
+        if let Some(m) = NonZeroU64::new(ver.minor) {
+            return SemverCompatibility::Minor(m);
+        }
+        SemverCompatibility::Patch(ver.patch)
+    }
+}
+
+impl ActivationKey {
+    /// Returns a value that implements a "no hash" hashable value.
+    /// This is possible since all `ActivationKey` are already interned in a `HashSet`.
+    pub fn no_hash(self) -> ActivationKeyNoHash {
+        ActivationKeyNoHash(std::ptr::from_ref(self.inner) as u64)
+    }
+}
+
+#[derive(Clone, Copy, Eq, PartialEq)]
+pub struct ActivationKeyNoHash(u64);
+
+impl nohash_hasher::IsEnabled for ActivationKeyNoHash {}
+
+impl Hash for ActivationKeyNoHash {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        state.write_u64(self.0);
+    }
+}

src/cargo/core/resolver/activation_key.rs

@@ -1,13 +1,13 @@
+use super::activation_key::ActivationKeyNoHash;
 use super::dep_cache::RegistryQueryer;
 use super::errors::ActivateResult;
 use super::types::{ConflictMap, ConflictReason, FeaturesSet, ResolveOpts};
 use super::RequestedFeatures;
-use crate::core::{Dependency, PackageId, SourceId, Summary};
+use crate::core::{Dependency, PackageId, Summary};
 use crate::util::interning::InternedString;
 use crate::util::Graph;
 use anyhow::format_err;
 use std::collections::HashMap;
-use std::num::NonZeroU64;
 use tracing::debug;
 
 // A `Context` is basically a bunch of local resolution information which is
@@ -22,56 +22,26 @@ pub struct ResolverContext {
     pub resolve_features: im_rc::HashMap<PackageId, FeaturesSet, rustc_hash::FxBuildHasher>,
     /// get the package that will be linking to a native library by its links attribute
     pub links: im_rc::HashMap<InternedString, PackageId, rustc_hash::FxBuildHasher>,
-
     /// a way to look up for a package in activations what packages required it
     /// and all of the exact deps that it fulfilled.
     pub parents: Graph<PackageId, im_rc::HashSet<Dependency, 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<
+    ActivationKeyNoHash,
+    (Summary, ContextAge),
+    nohash_hasher::BuildNoHashHasher<ActivationKeyNoHash>,
+>;
+
 /// 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
 /// of decisions made to get to this state.
 /// Several structures store the `ContextAge` when it was added,
 /// to be used in `find_candidate` for backtracking.
 pub type ContextAge = usize;
 
-/// 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);
-
-pub type Activations =
-    im_rc::HashMap<ActivationsKey, (Summary, ContextAge), rustc_hash::FxBuildHasher>;
-
-/// 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),
-    Minor(NonZeroU64),
-    Patch(u64),
-}
-
-impl From<&semver::Version> for SemverCompatibility {
-    fn from(ver: &semver::Version) -> Self {
-        if let Some(m) = NonZeroU64::new(ver.major) {
-            return SemverCompatibility::Major(m);
-        }
-        if let Some(m) = NonZeroU64::new(ver.minor) {
-            return SemverCompatibility::Minor(m);
-        }
-        SemverCompatibility::Patch(ver.patch)
-    }
-}
-
-impl PackageId {
-    pub fn as_activations_key(self) -> ActivationsKey {
-        (self.name(), self.source_id(), self.version().into())
-    }
-}
-
 impl ResolverContext {
     pub fn new() -> ResolverContext {
         ResolverContext {
@@ -98,7 +68,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().no_hash()) {
             im_rc::hashmap::Entry::Occupied(o) => {
                 debug_assert_eq!(
                     &o.get().0,
@@ -137,8 +107,13 @@ 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 prev = self.activations.insert(key, (summary.clone(), age));
+                        let new_id =
+                            PackageId::new(id.name(), id.version().clone(), dep.source_id());
+
+                        let prev = self
+                            .activations
+                            .insert(new_id.activation_key().no_hash(), (summary.clone(), age));
+
                         if let Some((previous_summary, _)) = prev {
                             return Err(
                                 (previous_summary.package_id(), ConflictReason::Semver).into()
@@ -181,9 +156,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().no_hash())?;
+
+        if summary.package_id() == id {
+            Some(*age)
+        } else {
+            None
+        }
     }
 
     /// Checks whether all of `parent` and the keys of `conflicting activations`
@@ -199,8 +178,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)
     }

src/cargo/core/resolver/context.rs

@@ -85,6 +85,7 @@ pub use self::resolve::{Resolve, ResolveVersion};
 pub use self::types::{ResolveBehavior, ResolveOpts};
 pub use self::version_prefs::{VersionOrdering, VersionPreferences};
 
+pub(crate) mod activation_key;
 mod conflict_cache;
 mod context;
 mod dep_cache;
@@ -198,8 +199,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.
@@ -775,7 +775,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().no_hash()) {
                 if *a != b {
                     conflicting_prev_active
                         .entry(a.package_id())