Auto merge of #5157 - Eh2406:more_interning, r=alexcrichton

More interning

This is a forward approach to interning. Specifically `Dependency` and `PackageId` store their names as `InternedString`s and leave that value interned as long as possible. The alternative is to make a new `interned_name` function. The advantage of this approach is that a number of places in the code are doing `deb.name() == pid.name()` and are now using the fast pointer compare instead of the string compare, without the code needing to change. The disadvantage is that lots of places need to call `deref` with `&*` to convert to an `&str` and sum need to use `.to_inner()` to get a `&'static str`.

In a test on #4810 (comment)
Before we got to 10000000 ticks in ~48 sec
After we got to 10000000 ticks in ~44 sec

src/cargo/core/dependency.rs

@@ -7,6 +7,7 @@ use semver::ReqParseError;
 use serde::ser;
 
 use core::{SourceId, Summary, PackageId};
+use core::interning::InternedString;
 use util::{Cfg, CfgExpr, Config};
 use util::errors::{CargoResult, CargoResultExt, CargoError};
 
@@ -20,7 +21,7 @@ pub struct Dependency {
 /// The data underlying a Dependency.
 #[derive(PartialEq, Eq, Hash, Ord, PartialOrd, Clone, Debug)]
 struct Inner {
- name: String,
+ name: InternedString,
  source_id: SourceId,
  registry_id: Option<SourceId>,
  req: VersionReq,
@@ -63,7 +64,7 @@ impl ser::Serialize for Dependency {
  where S: ser::Serializer,
  {
  SerializedDependency {
- name: self.name(),
+ name: &*self.name(),
  source: self.source_id(),
  req: self.version_req().to_string(),
  kind: self.kind(),
@@ -174,7 +175,7 @@ impl Dependency {
  pub fn new_override(name: &str, source_id: &SourceId) -> Dependency {
  Dependency {
  inner: Rc::new(Inner {
- name: name.to_string(),
+ name: InternedString::new(name),
  source_id: source_id.clone(),
  registry_id: None,
  req: VersionReq::any(),
@@ -194,8 +195,8 @@ impl Dependency {
  &self.inner.req
  }
 
- pub fn name(&self) -> &str {
- &self.inner.name
+ pub fn name(&self) -> InternedString {
+ self.inner.name
  }
 
  pub fn source_id(&self) -> &SourceId {

src/cargo/core/interning.rs

@@ -6,6 +6,7 @@ use std::str;
 use std::mem;
 use std::cmp::Ordering;
 use std::ops::Deref;
+use std::hash::{Hash, Hasher};
 
 pub fn leek(s: String) -> &'static str {
  let boxed = s.into_boxed_str();
@@ -23,7 +24,7 @@ lazy_static! {
  RwLock::new(HashSet::new());
 }
 
-#[derive(Eq, PartialEq, Hash, Clone, Copy)]
+#[derive(Eq, PartialEq, Clone, Copy)]
 pub struct InternedString {
  ptr: *const u8,
  len: usize,
@@ -39,30 +40,43 @@ impl InternedString {
  cache.insert(s);
  InternedString { ptr: s.as_ptr(), len: s.len() }
  }
+ pub fn to_inner(&self) -> &'static str {
+ unsafe {
+ let slice = slice::from_raw_parts(self.ptr, self.len);
+ &str::from_utf8_unchecked(slice)
+ }
+ }
 }
 
 impl Deref for InternedString {
  type Target = str;
 
  fn deref(&self) -> &'static str {
- unsafe {
- let slice = slice::from_raw_parts(self.ptr, self.len);
- &str::from_utf8_unchecked(slice)
- }
+ self.to_inner()
+ }
+}
+
+impl Hash for InternedString {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ self.to_inner().hash(state);
  }
 }
 
 impl fmt::Debug for InternedString {
  fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
- let str: &str = &*self;
- write!(f, "InternedString {{ {} }}", str)
+ write!(f, "InternedString {{ {} }}", self.to_inner())
+ }
+}
+
+impl fmt::Display for InternedString {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ write!(f, "{}", self.to_inner())
  }
 }
 
 impl Ord for InternedString {
  fn cmp(&self, other: &InternedString) -> Ordering {
- let str: &str = &*self;
- str.cmp(&*other)
+ self.to_inner().cmp(&*other)
  }
 }
 

src/cargo/core/manifest.rs

@@ -10,6 +10,7 @@ use url::Url;
 
 use core::{Dependency, PackageId, Summary, SourceId, PackageIdSpec};
 use core::{WorkspaceConfig, Epoch, Features, Feature};
+use core::interning::InternedString;
 use util::Config;
 use util::toml::TomlManifest;
 use util::errors::*;
@@ -301,7 +302,7 @@ impl Manifest {
  pub fn exclude(&self) -> &[String] { &self.exclude }
  pub fn include(&self) -> &[String] { &self.include }
  pub fn metadata(&self) -> &ManifestMetadata { &self.metadata }
- pub fn name(&self) -> &str { self.package_id().name() }
+ pub fn name(&self) -> InternedString { self.package_id().name() }
  pub fn package_id(&self) -> &PackageId { self.summary.package_id() }
  pub fn summary(&self) -> &Summary { &self.summary }
  pub fn targets(&self) -> &[Target] { &self.targets }

src/cargo/core/package.rs

@@ -11,6 +11,7 @@ use lazycell::LazyCell;
 
 use core::{Dependency, Manifest, PackageId, SourceId, Target};
 use core::{Summary, SourceMap};
+use core::interning::InternedString;
 use ops;
 use util::{Config, internal, lev_distance};
 use util::errors::{CargoResult, CargoResultExt};
@@ -55,7 +56,7 @@ impl ser::Serialize for Package {
  let description = manmeta.description.as_ref().map(String::as_ref);
 
  SerializedPackage {
- name: package_id.name(),
+ name: &*package_id.name(),
  version: &package_id.version().to_string(),
  id: package_id,
  license,
@@ -95,7 +96,7 @@ impl Package {
  /// Get the path to the manifest
  pub fn manifest_path(&self) -> &Path { &self.manifest_path }
  /// Get the name of the package
- pub fn name(&self) -> &str { self.package_id().name() }
+ pub fn name(&self) -> InternedString { self.package_id().name() }
  /// Get the PackageId object for the package (fully defines a package)
  pub fn package_id(&self) -> &PackageId { self.manifest.package_id() }
  /// Get the root folder of the package

src/cargo/core/package_id.rs

@@ -11,6 +11,7 @@ use serde::ser;
 
 use util::{CargoResult, ToSemver};
 use core::source::SourceId;
+use core::interning::InternedString;
 
 /// Identifier for a specific version of a package in a specific source.
 #[derive(Clone)]
@@ -20,7 +21,7 @@ pub struct PackageId {
 
 #[derive(PartialEq, PartialOrd, Eq, Ord)]
 struct PackageIdInner {
- name: String,
+ name: InternedString,
  version: semver::Version,
  source_id: SourceId,
 }
@@ -63,7 +64,7 @@ impl<'de> de::Deserialize<'de> for PackageId {
 
  Ok(PackageId {
  inner: Arc::new(PackageIdInner {
- name: name.to_string(),
+ name: InternedString::new(name),
  version,
  source_id,
  }),
@@ -102,21 +103,21 @@ impl PackageId {
  let v = version.to_semver()?;
  Ok(PackageId {
  inner: Arc::new(PackageIdInner {
- name: name.to_string(),
+ name: InternedString::new(name),
  version: v,
  source_id: sid.clone(),
  }),
  })
  }
 
- pub fn name(&self) -> &str { &self.inner.name }
+ pub fn name(&self) -> InternedString { self.inner.name }
  pub fn version(&self) -> &semver::Version { &self.inner.version }
  pub fn source_id(&self) -> &SourceId { &self.inner.source_id }
 
  pub fn with_precise(&self, precise: Option<String>) -> PackageId {
  PackageId {
  inner: Arc::new(PackageIdInner {
- name: self.inner.name.to_string(),
+ name: self.inner.name,
  version: self.inner.version.clone(),
  source_id: self.inner.source_id.with_precise(precise),
  }),
@@ -126,7 +127,7 @@ impl PackageId {
  pub fn with_source_id(&self, source: &SourceId) -> PackageId {
  PackageId {
  inner: Arc::new(PackageIdInner {
- name: self.inner.name.to_string(),
+ name: self.inner.name,
  version: self.inner.version.clone(),
  source_id: source.clone(),
  }),

src/cargo/core/package_id_spec.rs

@@ -115,7 +115,7 @@ impl PackageIdSpec {
  }
 
  pub fn matches(&self, package_id: &PackageId) -> bool {
- if self.name() != package_id.name() { return false }
+ if self.name() != &*package_id.name() { return false }
 
  if let Some(ref v) = self.version {
  if v != package_id.version() {

src/cargo/core/registry.rs

@@ -317,7 +317,7 @@ impl<'cfg> PackageRegistry<'cfg> {
  -> CargoResult<Option<Summary>> {
  for s in self.overrides.iter() {
  let src = self.sources.get_mut(s).unwrap();
- let dep = Dependency::new_override(dep.name(), s);
+ let dep = Dependency::new_override(&*dep.name(), s);
  let mut results = src.query_vec(&dep)?;
  if !results.is_empty() {
  return Ok(Some(results.remove(0)))
@@ -519,7 +519,7 @@ fn lock(locked: &LockedMap,
  patches: &HashMap<Url, Vec<PackageId>>,
  summary: Summary) -> Summary {
  let pair = locked.get(summary.source_id()).and_then(|map| {
- map.get(summary.name())
+ map.get(&*summary.name())
  }).and_then(|vec| {
  vec.iter().find(|&&(ref id, _)| id == summary.package_id())
  });
@@ -568,7 +568,7 @@ fn lock(locked: &LockedMap,
  // all known locked packages to see if they match this dependency.
  // If anything does then we lock it to that and move on.
  let v = locked.get(dep.source_id()).and_then(|map| {
- map.get(dep.name())
+ map.get(&*dep.name())
  }).and_then(|vec| {
  vec.iter().find(|&&(ref id, _)| dep.matches_id(id))
  });
@@ -593,7 +593,7 @@ fn lock(locked: &LockedMap,
  assert!(remaining.next().is_none());
  let patch_source = patch_id.source_id();
  let patch_locked = locked.get(patch_source).and_then(|m| {
- m.get(patch_id.name())
+ m.get(&*patch_id.name())
  }).map(|list| {
  list.iter().any(|&(ref id, _)| id == patch_id)
  }).unwrap_or(false);

src/cargo/core/resolver/mod.rs

@@ -1044,7 +1044,7 @@ fn activation_error(cx: &Context,
  for &(p, r) in links_errors.iter() {
  if let ConflictReason::Links(ref link) = *r {
  msg.push_str("\n\nthe package `");
- msg.push_str(dep.name());
+ msg.push_str(&*dep.name());
  msg.push_str("` links to the native library `");
  msg.push_str(link);
  msg.push_str("`, but it conflicts with a previous package which links to `");
@@ -1059,13 +1059,13 @@ fn activation_error(cx: &Context,
  for &(p, r) in features_errors.iter() {
  if let ConflictReason::MissingFeatures(ref features) = *r {
  msg.push_str("\n\nthe package `");
- msg.push_str(p.name());
+ msg.push_str(&*p.name());
  msg.push_str("` depends on `");
- msg.push_str(dep.name());
+ msg.push_str(&*dep.name());
  msg.push_str("`, with features: `");
  msg.push_str(features);
  msg.push_str("` but `");
- msg.push_str(dep.name());
+ msg.push_str(&*dep.name());
  msg.push_str("` does not have these features.\n");
  }
  // p == parent so the full path is redundant.
@@ -1082,7 +1082,7 @@ fn activation_error(cx: &Context,
  }
 
  msg.push_str("\n\nfailed to select a version for `");
- msg.push_str(dep.name());
+ msg.push_str(&*dep.name());
  msg.push_str("` which could resolve this conflict");
 
  return format_err!("{}", msg)
@@ -1274,7 +1274,7 @@ fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method)
  reqs.require_feature(key)?;
  }
  for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
- reqs.require_dependency(dep.name());
+ reqs.require_dependency(dep.name().to_inner());
  }
  }
  Method::Required { features: requested_features, .. } => {
@@ -1304,7 +1304,7 @@ impl Context {
  method: &Method) -> CargoResult<bool> {
  let id = summary.package_id();
  let prev = self.activations
- .entry((InternedString::new(id.name()), id.source_id().clone()))
+ .entry((id.name(), id.source_id().clone()))
  .or_insert_with(||Rc::new(Vec::new()));
  if !prev.iter().any(|c| c == summary) {
  self.resolve_graph.push(GraphNode::Add(id.clone()));
@@ -1365,13 +1365,13 @@ impl Context {
  }
 
  fn prev_active(&self, dep: &Dependency) -> &[Summary] {
- self.activations.get(&(InternedString::new(dep.name()), dep.source_id().clone()))
+ self.activations.get(&(dep.name(), dep.source_id().clone()))
  .map(|v| &v[..])
  .unwrap_or(&[])
  }
 
  fn is_active(&self, id: &PackageId) -> bool {
- self.activations.get(&(InternedString::new(id.name()), id.source_id().clone()))
+ self.activations.get(&(id.name(), id.source_id().clone()))
  .map(|v| v.iter().any(|s| s.package_id() == id))
  .unwrap_or(false)
  }
@@ -1397,12 +1397,12 @@ impl Context {
  // Next, collect all actually enabled dependencies and their features.
  for dep in deps {
  // Skip optional dependencies, but not those enabled through a feature
- if dep.is_optional() && !reqs.deps.contains_key(dep.name()) {
+ if dep.is_optional() && !reqs.deps.contains_key(&*dep.name()) {
  continue
  }
  // So we want this dependency. Move the features we want from `feature_deps`
  // to `ret`.
- let base = reqs.deps.remove(dep.name()).unwrap_or((false, vec![]));
+ let base = reqs.deps.remove(&*dep.name()).unwrap_or((false, vec![]));
  if !dep.is_optional() && base.0 {
  self.warnings.push(
  format!("Package `{}` does not have feature `{}`. It has a required dependency \

src/cargo/core/summary.rs

@@ -32,7 +32,7 @@ impl Summary {
  features: BTreeMap<String, Vec<String>>,
  links: Option<String>) -> CargoResult<Summary> {
  for dep in dependencies.iter() {
- if features.get(dep.name()).is_some() {
+ if features.get(&*dep.name()).is_some() {
  bail!("Features and dependencies cannot have the \
  same name: `{}`", dep.name())
  }
@@ -47,7 +47,7 @@ impl Summary {
  let dep = parts.next().unwrap();
  let is_reexport = parts.next().is_some();
  if !is_reexport && features.get(dep).is_some() { continue }
- match dependencies.iter().find(|d| d.name() == dep) {
+ match dependencies.iter().find(|d| &*d.name() == dep) {
  Some(d) => {
  if d.is_optional() || is_reexport { continue }
  bail!("Feature `{}` depends on `{}` which is not an \
@@ -78,7 +78,7 @@ impl Summary {
  }
 
  pub fn package_id(&self) -> &PackageId { &self.inner.package_id }
- pub fn name(&self) -> &str { self.package_id().name() }
+ pub fn name(&self) -> InternedString { self.package_id().name() }
  pub fn version(&self) -> &Version { self.package_id().version() }
  pub fn source_id(&self) -> &SourceId { self.package_id().source_id() }
  pub fn dependencies(&self) -> &[Dependency] { &self.inner.dependencies }

src/cargo/ops/cargo_doc.rs

@@ -54,7 +54,7 @@ pub fn doc(ws: &Workspace, options: &DocOptions) -> CargoResult<()> {
  bail!("Passing multiple packages and `open` is not supported.\n\
  Please re-run this command with `-p <spec>` where `<spec>` \
  is one of the following:\n {}",
- pkgs.iter().map(|p| p.name()).collect::<Vec<_>>().join("\n "));
+ pkgs.iter().map(|p| p.name().to_inner()).collect::<Vec<_>>().join("\n "));
  } else if pkgs.len() == 1 {
  pkgs[0].name().replace("-", "_")
  } else {

src/cargo/ops/cargo_generate_lockfile.rs

@@ -136,7 +136,7 @@ pub fn update_lockfile(ws: &Workspace, opts: &UpdateOptions)
  resolve: &'a Resolve) ->
  Vec<(Vec<&'a PackageId>, Vec<&'a PackageId>)> {
  fn key(dep: &PackageId) -> (&str, &SourceId) {
- (dep.name(), dep.source_id())
+ (dep.name().to_inner(), dep.source_id())
  }
 
  // Removes all package ids in `b` from `a`. Note that this is somewhat