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

a faster hash for ActivationsKey #14915

Merged
merged 6 commits into from
Dec 10, 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
38 changes: 4 additions & 34 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
use super::dep_cache::RegistryQueryer;
use super::errors::ActivateResult;
use super::types::{ConflictMap, ConflictReason, FeaturesSet, ResolveOpts};
use super::types::{ActivationsKey, 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
Expand Down Expand Up @@ -39,39 +38,9 @@ pub type ContextAge = usize;
/// 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 {
Expand Down Expand Up @@ -137,7 +106,8 @@ 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 =
ActivationsKey::new(id.name(), id.version().into(), dep.source_id());
let prev = self.activations.insert(key, (summary.clone(), age));
if let Some((previous_summary, _)) = prev {
return Err(
Expand Down
55 changes: 54 additions & 1 deletion src/cargo/core/resolver/types.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use super::features::{CliFeatures, RequestedFeatures};
use crate::core::{Dependency, PackageId, Summary};
use crate::core::{Dependency, PackageId, SourceId, Summary};
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::GlobalContext;
use std::cmp::Ordering;
use std::collections::{BTreeMap, BTreeSet};
use std::num::NonZeroU64;
use std::rc::Rc;
use std::time::{Duration, Instant};

Expand Down Expand Up @@ -163,6 +164,58 @@ impl ResolveOpts {
}
}

/// A key that when stord in a hash map ensures that there is only one
/// semver compatible version of each crate.
/// Find the activated version of a crate based on the name, source, and semver compatibility.
#[derive(Clone, PartialEq, Eq, Debug, Ord, PartialOrd)]
pub struct ActivationsKey(InternedString, SemverCompatibility, SourceId);

impl ActivationsKey {
pub fn new(
name: InternedString,
ver: SemverCompatibility,
source_id: SourceId,
) -> ActivationsKey {
ActivationsKey(name, ver, source_id)
}
}

impl std::hash::Hash for ActivationsKey {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.0.hash(state);
self.1.hash(state);
// self.2.hash(state); // Packages that only differ by SourceId are rare enough to not be worth hashing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have 3 improvments

  • Re-order
  • Pointer hash on the name
  • Skipping the source hash

Do we know how much of an improvement each one of these is? I'm mostly wondering about skipping the source hash as that feels icky for some reason.

Not going to give this feeling too much weight though. I'll mostly defer to you on this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know how much of an improvement each one of these is?

I just collected overall performance numbers, I will try and get you some intermediate numbers.

I'm mostly wondering about skipping the source hash as that feels icky for some reason.

Can you articulate why? HashMaps can devolve to O(n) if skip fields are the only thing distinguishing n items, but we specifically commented to record why we didn't think that would happen. Skipping a field when hashing is a pretty standard performance trick.

}
}

/// 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 {
ActivationsKey(self.name(), self.version().into(), self.source_id())
}
}

#[derive(Clone)]
pub struct DepsFrame {
pub parent: Summary,
Expand Down
Loading