Auto merge of #7310 - Eh2406:minimal-copy, r=alexcrichton

minimal-copy `deserialize` for `InternedString`

I just learnt that `serde::Deserialize` for `Cow<'a, str>` allocates by default! Thus negating the intended benefit of ea957da, and this is in the hot loop for no-op builds #6908. The docs https://serde.rs/lifetimes.html#borrowing-data-in-a-derived-impl say you can fix this with a `#[serde(borrow)]`, but in practice this does not work on  `Option<Cow<'a, str>>`.  Some of these are just going to be turned into `InternedString`s, so we can tell serde to do that directly saving an allocation while we are at it!

So is this faster, or just reducing the number of `InternedString` <-> `&str` conversions?
I ran the benchmark script developed for #7168 (comment). Looks like no change for Cargo's lockfile and a ~7% improvement for the 2000 crate stress test.

Author: bors

crates/resolver-tests/src/lib.rs

@@ -171,7 +171,7 @@ pub fn resolve_with_config_raw(
         pkg_id("root"),
         deps,
         &BTreeMap::<String, Vec<String>>::new(),
-        None::<String>,
+        None::<&String>,
         false,
     )
     .unwrap();

crates/resolver-tests/tests/resolve.rs

@@ -1,5 +1,3 @@
-use std::env;
-
 use cargo::core::dependency::Kind;
 use cargo::core::{enable_nightly_features, Dependency};
 use cargo::util::{is_ci, Config};

src/cargo/core/dependency.rs

@@ -101,7 +101,7 @@ pub enum Kind {
 }
 
 fn parse_req_with_deprecated(
-    name: &str,
+    name: InternedString,
     req: &str,
     extra: Option<(PackageId, &Config)>,
 ) -> CargoResult<VersionReq> {
@@ -163,12 +163,13 @@ impl ser::Serialize for Kind {
 impl Dependency {
     /// Attempt to create a `Dependency` from an entry in the manifest.
     pub fn parse(
-        name: &str,
+        name: impl Into<InternedString>,
         version: Option<&str>,
         source_id: SourceId,
         inside: PackageId,
         config: &Config,
     ) -> CargoResult<Dependency> {
+        let name = name.into();
         let arg = Some((inside, config));
         let (specified_req, version_req) = match version {
             Some(v) => (true, parse_req_with_deprecated(name, v, arg)?),
@@ -187,10 +188,11 @@ impl Dependency {
 
     /// Attempt to create a `Dependency` from an entry in the manifest.
     pub fn parse_no_deprecated(
-        name: &str,
+        name: impl Into<InternedString>,
         version: Option<&str>,
         source_id: SourceId,
     ) -> CargoResult<Dependency> {
+        let name = name.into();
         let (specified_req, version_req) = match version {
             Some(v) => (true, parse_req_with_deprecated(name, v, None)?),
             None => (false, VersionReq::any()),
@@ -206,11 +208,11 @@ impl Dependency {
         Ok(ret)
     }
 
-    pub fn new_override(name: &str, source_id: SourceId) -> Dependency {
+    pub fn new_override(name: InternedString, source_id: SourceId) -> Dependency {
         assert!(!name.is_empty());
         Dependency {
             inner: Rc::new(Inner {
-                name: InternedString::new(name),
+                name,
                 source_id,
                 registry_id: None,
                 req: VersionReq::any(),
@@ -338,12 +340,9 @@ impl Dependency {
     /// Sets the list of features requested for the package.
     pub fn set_features(
         &mut self,
-        features: impl IntoIterator<Item = impl AsRef<str>>,
+        features: impl IntoIterator<Item = impl Into<InternedString>>,
     ) -> &mut Dependency {
-        Rc::make_mut(&mut self.inner).features = features
-            .into_iter()
-            .map(|s| InternedString::new(s.as_ref()))
-            .collect();
+        Rc::make_mut(&mut self.inner).features = features.into_iter().map(|s| s.into()).collect();
         self
     }
 
@@ -376,8 +375,11 @@ impl Dependency {
         self
     }
 
-    pub fn set_explicit_name_in_toml(&mut self, name: &str) -> &mut Dependency {
-        Rc::make_mut(&mut self.inner).explicit_name_in_toml = Some(InternedString::new(name));
+    pub fn set_explicit_name_in_toml(
+        &mut self,
+        name: impl Into<InternedString>,
+    ) -> &mut Dependency {
+        Rc::make_mut(&mut self.inner).explicit_name_in_toml = Some(name.into());
         self
     }
 

src/cargo/core/interning.rs

@@ -23,6 +23,18 @@ pub struct InternedString {
     inner: &'static str,
 }
 
+impl<'a> From<&'a str> for InternedString {
+    fn from(item: &'a str) -> Self {
+        InternedString::new(item)
+    }
+}
+
+impl<'a> From<&'a String> for InternedString {
+    fn from(item: &'a String) -> Self {
+        InternedString::new(item)
+    }
+}
+
 impl PartialEq for InternedString {
     fn eq(&self, other: &InternedString) -> bool {
         ptr::eq(self.as_str(), other.as_str())
@@ -56,6 +68,12 @@ impl Deref for InternedString {
     }
 }
 
+impl AsRef<str> for InternedString {
+    fn as_ref(&self) -> &str {
+        self.as_str()
+    }
+}
+
 impl Hash for InternedString {
     // N.B., we can't implement this as `identity(self).hash(state)`,
     // because we use this for on-disk fingerprints and so need
@@ -105,3 +123,29 @@ impl Serialize for InternedString {
         serializer.serialize_str(self.inner)
     }
 }
+
+struct InternedStringVisitor;
+
+impl<'de> serde::Deserialize<'de> for InternedString {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        deserializer.deserialize_str(InternedStringVisitor)
+    }
+}
+
+impl<'de> serde::de::Visitor<'de> for InternedStringVisitor {
+    type Value = InternedString;
+
+    fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        formatter.write_str("an String like thing")
+    }
+
+    fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+    where
+        E: serde::de::Error,
+    {
+        Ok(InternedString::new(v))
+    }
+}

src/cargo/core/package_id.rs

@@ -113,9 +113,13 @@ impl Hash for PackageId {
 }
 
 impl PackageId {
-    pub fn new<T: ToSemver>(name: &str, version: T, sid: SourceId) -> CargoResult<PackageId> {
+    pub fn new<T: ToSemver>(
+        name: impl Into<InternedString>,
+        version: T,
+        sid: SourceId,
+    ) -> CargoResult<PackageId> {
         let v = version.to_semver()?;
-        Ok(PackageId::pure(InternedString::new(name), v, sid))
+        Ok(PackageId::pure(name.into(), v, sid))
     }
 
     pub fn pure(name: InternedString, version: semver::Version, source_id: SourceId) -> PackageId {

src/cargo/core/registry.rs

@@ -366,7 +366,7 @@ impl<'cfg> PackageRegistry<'cfg> {
     fn query_overrides(&mut self, dep: &Dependency) -> CargoResult<Option<Summary>> {
         for &s in self.overrides.iter() {
             let src = self.sources.get_mut(s).unwrap();
-            let dep = Dependency::new_override(&*dep.package_name(), s);
+            let dep = Dependency::new_override(dep.package_name(), s);
             let mut results = src.query_vec(&dep)?;
             if !results.is_empty() {
                 return Ok(Some(results.remove(0)));

src/cargo/core/resolver/dep_cache.rs

@@ -433,7 +433,7 @@ impl Requirements<'_> {
         for fv in self
             .summary
             .features()
-            .get(feat.as_str())
+            .get(&feat)
             .expect("must be a valid feature")
         {
             match *fv {

src/cargo/core/summary.rs

@@ -37,7 +37,7 @@ impl Summary {
         pkg_id: PackageId,
         dependencies: Vec<Dependency>,
         features: &BTreeMap<K, Vec<impl AsRef<str>>>,
-        links: Option<impl AsRef<str>>,
+        links: Option<impl Into<InternedString>>,
         namespaced_features: bool,
     ) -> CargoResult<Summary>
     where
@@ -66,7 +66,7 @@ impl Summary {
                 dependencies,
                 features: feature_map,
                 checksum: None,
-                links: links.map(|l| InternedString::new(l.as_ref())),
+                links: links.map(|l| l.into()),
                 namespaced_features,
             }),
         })

src/cargo/sources/registry/index.rs

@@ -716,7 +716,7 @@ impl IndexSummary {
             links,
         } = serde_json::from_slice(line)?;
         log::trace!("json parsed registry {}/{}", name, vers);
-        let pkgid = PackageId::new(&name, &vers, source_id)?;
+        let pkgid = PackageId::new(name, &vers, source_id)?;
         let deps = deps
             .into_iter()
             .map(|dep| dep.into_dep(source_id))

src/cargo/sources/registry/mod.rs

@@ -219,17 +219,18 @@ pub struct RegistryConfig {
 
 #[derive(Deserialize)]
 pub struct RegistryPackage<'a> {
-    name: Cow<'a, str>,
+    name: InternedString,
     vers: Version,
+    #[serde(borrow)]
     deps: Vec<RegistryDependency<'a>>,
-    features: BTreeMap<Cow<'a, str>, Vec<Cow<'a, str>>>,
+    features: BTreeMap<InternedString, Vec<InternedString>>,
     cksum: String,
     yanked: Option<bool>,
-    links: Option<Cow<'a, str>>,
+    links: Option<InternedString>,
 }
 
 #[test]
-fn escaped_cher_in_json() {
+fn escaped_char_in_json() {
     let _: RegistryPackage<'_> = serde_json::from_str(
         r#"{"name":"a","vers":"0.0.1","deps":[],"cksum":"bae3","features":{}}"#,
     )
@@ -275,15 +276,16 @@ enum Field {
 
 #[derive(Deserialize)]
 struct RegistryDependency<'a> {
-    name: Cow<'a, str>,
+    name: InternedString,
+    #[serde(borrow)]
     req: Cow<'a, str>,
-    features: Vec<Cow<'a, str>>,
+    features: Vec<InternedString>,
     optional: bool,
     default_features: bool,
     target: Option<Cow<'a, str>>,
     kind: Option<Cow<'a, str>>,
     registry: Option<Cow<'a, str>>,
-    package: Option<Cow<'a, str>>,
+    package: Option<InternedString>,
     public: Option<bool>,
 }
 
@@ -309,10 +311,9 @@ impl<'a> RegistryDependency<'a> {
             default
         };
 
-        let mut dep =
-            Dependency::parse_no_deprecated(package.as_ref().unwrap_or(&name), Some(&req), id)?;
+        let mut dep = Dependency::parse_no_deprecated(package.unwrap_or(name), Some(&req), id)?;
         if package.is_some() {
-            dep.set_explicit_name_in_toml(&name);
+            dep.set_explicit_name_in_toml(name);
         }
         let kind = match kind.as_ref().map(|s| &s[..]).unwrap_or("") {
             "dev" => Kind::Development,