Auto merge of #6332 - dwijnand:intern-more, r=Eh2406

Intern PackageId

Refs #6207

src/cargo/core/package_id.rs

@@ -1,9 +1,10 @@
-use std::cmp::Ordering;
+use std::collections::HashSet;
 use std::fmt::{self, Formatter};
 use std::hash;
 use std::hash::Hash;
 use std::path::Path;
-use std::sync::Arc;
+use std::ptr;
+use std::sync::Mutex;
 
 use semver;
 use serde::de;
@@ -13,19 +14,41 @@ use core::interning::InternedString;
 use core::source::SourceId;
 use util::{CargoResult, ToSemver};
 
+lazy_static! {
+    static ref PACKAGE_ID_CACHE: Mutex<HashSet<&'static PackageIdInner>> = Mutex::new(HashSet::new());
+}
+
 /// Identifier for a specific version of a package in a specific source.
-#[derive(Clone)]
+#[derive(Clone, Copy, Eq, PartialOrd, Ord)]
 pub struct PackageId {
-    inner: Arc<PackageIdInner>,
+    inner: &'static PackageIdInner,
 }
 
-#[derive(PartialEq, PartialOrd, Eq, Ord)]
+#[derive(PartialOrd, Eq, Ord)]
 struct PackageIdInner {
     name: InternedString,
     version: semver::Version,
     source_id: SourceId,
 }
 
+// Custom equality that uses full equality of SourceId, rather than its custom equality.
+impl PartialEq for PackageIdInner {
+    fn eq(&self, other: &Self) -> bool {
+        self.name == other.name
+            && self.version == other.version
+            && self.source_id.full_eq(&other.source_id)
+    }
+}
+
+// Custom hash that is coherent with the custom equality above.
+impl Hash for PackageIdInner {
+    fn hash<S: hash::Hasher>(&self, into: &mut S) {
+        self.name.hash(into);
+        self.version.hash(into);
+        self.source_id.full_hash(into);
+    }
+}
+
 impl ser::Serialize for PackageId {
     fn serialize<S>(&self, s: S) -> Result<S::Ok, S::Error>
     where
@@ -64,51 +87,56 @@ impl<'de> de::Deserialize<'de> for PackageId {
         };
         let source_id = SourceId::from_url(url).map_err(de::Error::custom)?;
 
-        Ok(PackageId {
-            inner: Arc::new(PackageIdInner {
+        Ok(PackageId::wrap(
+            PackageIdInner {
                 name: InternedString::new(name),
                 version,
                 source_id,
-            }),
-        })
-    }
-}
-
-impl Hash for PackageId {
-    fn hash<S: hash::Hasher>(&self, state: &mut S) {
-        self.inner.name.hash(state);
-        self.inner.version.hash(state);
-        self.inner.source_id.hash(state);
+            }
+        ))
     }
 }
 
 impl PartialEq for PackageId {
     fn eq(&self, other: &PackageId) -> bool {
-        (*self.inner).eq(&*other.inner)
-    }
-}
-impl PartialOrd for PackageId {
-    fn partial_cmp(&self, other: &PackageId) -> Option<Ordering> {
-        (*self.inner).partial_cmp(&*other.inner)
+        if ptr::eq(self.inner, other.inner) {
+            return true;
+        }
+        self.inner.name == other.inner.name
+            && self.inner.version == other.inner.version
+            && self.inner.source_id == other.inner.source_id
     }
 }
-impl Eq for PackageId {}
-impl Ord for PackageId {
-    fn cmp(&self, other: &PackageId) -> Ordering {
-        (*self.inner).cmp(&*other.inner)
+
+impl<'a> Hash for PackageId {
+    fn hash<S: hash::Hasher>(&self, state: &mut S) {
+        self.inner.name.hash(state);
+        self.inner.version.hash(state);
+        self.inner.source_id.hash(state);
     }
 }
 
 impl PackageId {
     pub fn new<T: ToSemver>(name: &str, version: T, sid: SourceId) -> CargoResult<PackageId> {
         let v = version.to_semver()?;
-        Ok(PackageId {
-            inner: Arc::new(PackageIdInner {
+
+        Ok(PackageId::wrap(
+            PackageIdInner {
                 name: InternedString::new(name),
                 version: v,
                 source_id: sid,
-            }),
-        })
+            }
+        ))
+    }
+
+    fn wrap(inner: PackageIdInner) -> PackageId {
+        let mut cache = PACKAGE_ID_CACHE.lock().unwrap();
+        let inner = cache.get(&inner).map(|&x| x).unwrap_or_else(|| {
+            let inner = Box::leak(Box::new(inner));
+            cache.insert(inner);
+            inner
+        });
+        PackageId { inner }
     }
 
     pub fn name(&self) -> InternedString {
@@ -122,23 +150,23 @@ impl PackageId {
     }
 
     pub fn with_precise(&self, precise: Option<String>) -> PackageId {
-        PackageId {
-            inner: Arc::new(PackageIdInner {
+        PackageId::wrap(
+            PackageIdInner {
                 name: self.inner.name,
                 version: self.inner.version.clone(),
                 source_id: self.inner.source_id.with_precise(precise),
-            }),
-        }
+            }
+        )
     }
 
     pub fn with_source_id(&self, source: SourceId) -> PackageId {
-        PackageId {
-            inner: Arc::new(PackageIdInner {
+        PackageId::wrap(
+            PackageIdInner {
                 name: self.inner.name,
                 version: self.inner.version.clone(),
                 source_id: source,
-            }),
-        }
+            }
+        )
     }
 
     pub fn stable_hash<'a>(&'a self, workspace: &'a Path) -> PackageIdStableHash<'a> {
@@ -195,4 +223,27 @@ mod tests {
         assert!(PackageId::new("foo", "bar", repo).is_err());
         assert!(PackageId::new("foo", "", repo).is_err());
     }
+
+    #[test]
+    fn debug() {
+        let loc = CRATES_IO_INDEX.to_url().unwrap();
+        let pkg_id = PackageId::new("foo", "1.0.0", SourceId::for_registry(&loc).unwrap()).unwrap();
+        assert_eq!(r#"PackageId { name: "foo", version: "1.0.0", source: "registry `https://github.com/rust-lang/crates.io-index`" }"#, format!("{:?}", pkg_id));
+
+        let pretty = r#"
+PackageId {
+    name: "foo",
+    version: "1.0.0",
+    source: "registry `https://github.com/rust-lang/crates.io-index`"
+}
+"#.trim();
+        assert_eq!(pretty, format!("{:#?}", pkg_id));
+    }
+
+    #[test]
+    fn display() {
+        let loc = CRATES_IO_INDEX.to_url().unwrap();
+        let pkg_id = PackageId::new("foo", "1.0.0", SourceId::for_registry(&loc).unwrap()).unwrap();
+        assert_eq!("foo v1.0.0", pkg_id.to_string());
+    }
 }

src/cargo/core/source/source_id.rs

@@ -335,6 +335,14 @@ impl SourceId {
         }
         self.hash(into)
     }
+
+    pub fn full_eq(&self, other: &SourceId) -> bool {
+        ptr::eq(self.inner, other.inner)
+    }
+
+    pub fn full_hash<S: hash::Hasher>(&self, into: &mut S) {
+        ptr::NonNull::from(self.inner).hash(into)
+    }
 }
 
 impl PartialOrd for SourceId {

tests/testsuite/git.rs

@@ -11,6 +11,7 @@ use std::thread;
 use support::paths::{self, CargoPathExt};
 use support::sleep_ms;
 use support::{basic_lib_manifest, basic_manifest, git, main_file, path2url, project};
+use support::Project;
 
 #[test]
 fn cargo_compile_simple_git_dep() {
@@ -1090,6 +1091,18 @@ fn two_deps_only_update_one() {
         ).file("src/main.rs", "fn main() {}")
         .build();
 
+    fn oid_to_short_sha(oid: git2::Oid) -> String {
+        oid.to_string()[..8].to_string()
+    }
+    fn git_repo_head_sha(p: &Project) -> String {
+        let repo = git2::Repository::open(p.root()).unwrap();
+        let head = repo.head().unwrap().target().unwrap();
+        oid_to_short_sha(head)
+    }
+
+    println!("dep1 head sha: {}", git_repo_head_sha(&git1));
+    println!("dep2 head sha: {}", git_repo_head_sha(&git2));
+
     p.cargo("build")
         .with_stderr(
             "[UPDATING] git repository `[..]`\n\
@@ -1106,7 +1119,8 @@ fn two_deps_only_update_one() {
         .unwrap();
     let repo = git2::Repository::open(&git1.root()).unwrap();
     git::add(&repo);
-    git::commit(&repo);
+    let oid = git::commit(&repo);
+    println!("dep1 head sha: {}", oid_to_short_sha(oid));
 
     p.cargo("update -p dep1")
         .with_stderr(&format!(