Auto merge of #5293 - Eh2406:InternMoreStrings, r=alexcrichton

bors · bors · commit 29d996043e38 · 2018-04-04T19:49:20.000Z
Intern more strings As pointed out in #5270 (comment), that clean up adds the mildly expensive `InternedString::new` to the hot path in the resolver. The process of this PR is: 1. Find a `InternedString::new` in the hot path. 2. replace the argument of type `&str` that is passed along to `InternedString::new` with the type `InternedString` 3. add an `InternedString::new` to the callers until it type checked. 4. Repeat from step 1. This stop if: - It was traced back to something that was already an `InternedString` - It was traced back to a `.to_string()` call - It was in a persistent object creation cc: - @djc this is building on your work, I don't want to get in your way. - @alexcrichton is this making things clearer and do you want to see a performance check?
diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs
@@ -32,7 +32,7 @@ struct Inner {
 
     optional: bool,
     default_features: bool,
-    features: Vec<String>,
+    features: Vec<InternedString>,
 
     // This dependency should be used only for this platform.
     // `None` means *all platforms*.
@@ -64,14 +64,15 @@ impl ser::Serialize for Dependency {
     where
         S: ser::Serializer,
     {
+        let string_features: Vec<_> = self.features().iter().map(|s| s.to_string()).collect();
         SerializedDependency {
             name: &*self.name(),
             source: self.source_id(),
             req: self.version_req().to_string(),
             kind: self.kind(),
             optional: self.is_optional(),
             uses_default_features: self.uses_default_features(),
-            features: self.features(),
+            features: &string_features,
             target: self.platform(),
             rename: self.rename(),
         }.serialize(s)
@@ -250,7 +251,8 @@ impl Dependency {
 
     /// Sets the list of features requested for the package.
     pub fn set_features(&mut self, features: Vec<String>) -> &mut Dependency {
-        Rc::make_mut(&mut self.inner).features = features;
+        Rc::make_mut(&mut self.inner).features =
+            features.iter().map(|s| InternedString::new(s)).collect();
         self
     }
 
@@ -334,7 +336,7 @@ impl Dependency {
         self.inner.default_features
     }
     /// Returns the list of features that are requested by the dependency.
-    pub fn features(&self) -> &[String] {
+    pub fn features(&self) -> &[InternedString] {
         &self.inner.features
     }
 
diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs
@@ -1,13 +1,13 @@
 use std::collections::{HashMap, HashSet};
 use std::rc::Rc;
 
-use core::{Dependency, FeatureValue, PackageId, SourceId, Summary};
 use core::interning::InternedString;
-use util::Graph;
+use core::{Dependency, FeatureValue, PackageId, SourceId, Summary};
 use util::CargoResult;
+use util::Graph;
 
-use super::types::{ActivateResult, ConflictReason, DepInfo, GraphNode, Method, RcList};
 use super::types::RegistryQueryer;
+use super::types::{ActivateResult, ConflictReason, DepInfo, GraphNode, Method, RcList};
 
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use super::encode::{Metadata, WorkspaceResolve};
@@ -160,7 +160,7 @@ impl Context {
         parent: Option<&Summary>,
         s: &'b Summary,
         method: &'b Method,
-    ) -> ActivateResult<Vec<(Dependency, Vec<String>)>> {
+    ) -> ActivateResult<Vec<(Dependency, Vec<InternedString>)>> {
         let dev_deps = match *method {
             Method::Everything => true,
             Method::Required { dev_deps, .. } => dev_deps,
@@ -182,7 +182,7 @@ impl Context {
         {
             requested
                 .iter()
-                .map(|f| FeatureValue::new(f, s))
+                .map(|&f| FeatureValue::new(f, s))
                 .collect::<Vec<FeatureValue>>()
         } else {
             vec![]
@@ -210,7 +210,7 @@ impl Context {
                 ));
             }
             let mut base = base.1;
-            base.extend(dep.features().iter().cloned());
+            base.extend(dep.features().iter());
             for feature in base.iter() {
                 if feature.contains('/') {
                     return Err(
@@ -331,7 +331,7 @@ struct Requirements<'a> {
     // specified set of features enabled. The boolean indicates whether this
     // package was specifically requested (rather than just requesting features
     // *within* this package).
-    deps: HashMap<&'a str, (bool, Vec<String>)>,
+    deps: HashMap<&'a str, (bool, Vec<InternedString>)>,
     // The used features set is the set of features which this local package had
     // enabled, which is later used when compiling to instruct the code what
     // features were enabled.
@@ -349,13 +349,13 @@ impl<'r> Requirements<'r> {
         }
     }
 
-    fn require_crate_feature(&mut self, package: &'r str, feat: &'r str) {
+    fn require_crate_feature(&mut self, package: &'r str, feat: InternedString) {
         self.used.insert(package);
         self.deps
             .entry(package)
             .or_insert((false, Vec::new()))
             .1
-            .push(feat.to_string());
+            .push(feat);
     }
 
     fn seen(&mut self, feat: &'r str) -> bool {
@@ -399,7 +399,7 @@ impl<'r> Requirements<'r> {
         match *fv {
             FeatureValue::Feature(ref feat) => self.require_feature(feat),
             FeatureValue::Crate(ref dep) => Ok(self.require_dependency(dep)),
-            FeatureValue::CrateFeature(ref dep, ref dep_feat) => {
+            FeatureValue::CrateFeature(ref dep, dep_feat) => {
                 Ok(self.require_crate_feature(dep, dep_feat))
             }
         }
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
@@ -56,6 +56,7 @@ use semver;
 
 use core::{Dependency, PackageId, Registry, Summary};
 use core::PackageIdSpec;
+use core::interning::InternedString;
 use util::config::Config;
 use util::Graph;
 use util::errors::{CargoError, CargoResult};
@@ -670,7 +671,7 @@ struct BacktrackFrame {
     remaining_candidates: RemainingCandidates,
     parent: Summary,
     dep: Dependency,
-    features: Rc<Vec<String>>,
+    features: Rc<Vec<InternedString>>,
     conflicting_activations: HashMap<PackageId, ConflictReason>,
 }
 
diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs
@@ -4,6 +4,7 @@ use std::ops::Range;
 use std::rc::Rc;
 
 use core::{Dependency, PackageId, PackageIdSpec, Registry, Summary};
+use core::interning::InternedString;
 use util::{CargoError, CargoResult};
 
 pub struct RegistryQueryer<'a> {
@@ -159,21 +160,21 @@ pub enum Method<'a> {
     Everything, // equivalent to Required { dev_deps: true, all_features: true, .. }
     Required {
         dev_deps: bool,
-        features: &'a [String],
+        features: &'a [InternedString],
         all_features: bool,
         uses_default_features: bool,
     },
 }
 
 impl<'r> Method<'r> {
-    pub fn split_features(features: &[String]) -> Vec<String> {
+    pub fn split_features(features: &[String]) -> Vec<InternedString> {
         features
             .iter()
             .flat_map(|s| s.split_whitespace())
             .flat_map(|s| s.split(','))
             .filter(|s| !s.is_empty())
-            .map(|s| s.to_string())
-            .collect::<Vec<String>>()
+            .map(|s| InternedString::new(s))
+            .collect::<Vec<InternedString>>()
     }
 }
 
@@ -245,7 +246,7 @@ impl Ord for DepsFrame {
 // Information about the dependencies for a crate, a tuple of:
 //
 // (dependency info, candidates, features activated)
-pub type DepInfo = (Dependency, Rc<Vec<Candidate>>, Rc<Vec<String>>);
+pub type DepInfo = (Dependency, Rc<Vec<Candidate>>, Rc<Vec<InternedString>>);
 
 pub type ActivateResult<T> = Result<T, ActivateError>;
 
diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs
@@ -2,12 +2,11 @@ use std::collections::BTreeMap;
 use std::mem;
 use std::rc::Rc;
 
-use semver::Version;
-
 use serde::{Serialize, Serializer};
 
-use core::{Dependency, PackageId, SourceId};
 use core::interning::InternedString;
+use core::{Dependency, PackageId, SourceId};
+use semver::Version;
 
 use util::CargoResult;
 
@@ -138,7 +137,7 @@ fn build_feature_map(
     for (feature, list) in features.iter() {
         let mut values = vec![];
         for dep in list {
-            let val = FeatureValue::build(dep, |fs| (&features).get(fs).is_some());
+            let val = FeatureValue::build(InternedString::new(dep), |fs| features.contains_key(fs));
             if let &Feature(_) = &val {
                 values.push(val);
                 continue;
@@ -201,7 +200,7 @@ pub enum FeatureValue {
 }
 
 impl FeatureValue {
-    fn build<T>(feature: &str, is_feature: T) -> FeatureValue
+    fn build<T>(feature: InternedString, is_feature: T) -> FeatureValue
     where
         T: Fn(&str) -> bool,
     {
@@ -211,13 +210,13 @@ impl FeatureValue {
                 let dep_feat = &dep_feat[1..];
                 FeatureValue::CrateFeature(InternedString::new(dep), InternedString::new(dep_feat))
             }
-            None if is_feature(&feature) => FeatureValue::Feature(InternedString::new(feature)),
-            None => FeatureValue::Crate(InternedString::new(feature)),
+            None if is_feature(&feature) => FeatureValue::Feature(feature),
+            None => FeatureValue::Crate(feature),
         }
     }
 
-    pub fn new(feature: &str, s: &Summary) -> FeatureValue {
-        Self::build(feature, |fs| s.features().get(fs).is_some())
+    pub fn new(feature: InternedString, s: &Summary) -> FeatureValue {
+        Self::build(feature, |fs| s.features().contains_key(fs))
     }
 
     pub fn to_string(&self) -> String {
diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs
@@ -171,7 +171,7 @@ fn transmit(
                 optional: dep.is_optional(),
                 default_features: dep.uses_default_features(),
                 name: dep.name().to_string(),
-                features: dep.features().to_vec(),
+                features: dep.features().iter().map(|s| s.to_string()).collect(),
                 version_req: dep.version_req().to_string(),
                 target: dep.platform().map(|s| s.to_string()),
                 kind: match dep.kind() {
diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs
@@ -472,9 +472,11 @@ fn register_previous_locks<'a>(
             //       dependency on that crate to enable the feature. For now
             //       this bug is better than the always updating registry
             //       though...
-            if !ws.members().any(|pkg| pkg.package_id() == member.package_id()) &&
-                (dep.is_optional() || !dep.is_transitive()) {
-                continue
+            if !ws.members()
+                .any(|pkg| pkg.package_id() == member.package_id())
+                && (dep.is_optional() || !dep.is_transitive())
+            {
+                continue;
             }
 
             // Ok if nothing matches, then we poison the source of this
