add lots of comments

src/cargo/core/resolver/context.rs

@@ -1,4 +1,4 @@
-use std::collections::{BTreeSet, HashMap};
+use std::collections::HashMap;
 use std::num::NonZeroU64;
 use std::rc::Rc;
 
@@ -13,7 +13,7 @@ use crate::util::CargoResult;
 use crate::util::Graph;
 
 use super::dep_cache::RegistryQueryer;
-use super::types::{ConflictMap, Method};
+use super::types::{ConflictMap, FeaturesSet, Method};
 
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use super::encode::{Metadata, WorkspaceResolve};
@@ -27,7 +27,7 @@ pub use super::resolve::Resolve;
 pub struct Context {
     pub activations: Activations,
     /// list the features that are activated for each package
-    pub resolve_features: im_rc::HashMap<PackageId, Rc<BTreeSet<InternedString>>>,
+    pub resolve_features: im_rc::HashMap<PackageId, FeaturesSet>,
     /// get the package that will be linking to a native library by its links attribute
     pub links: im_rc::HashMap<InternedString, PackageId>,
     /// for each package the list of names it can see,

src/cargo/core/resolver/dep_cache.rs

@@ -1,3 +1,14 @@
+//! There are 2 sources of facts for the resolver:
+//!
+//! - The Registry tells us for a Dependency what versions are available to fulfil it.
+//! - The Summary tells us for a version (and features) what dependencies need to be fulfilled for it to be activated.
+//!
+//! These constitute immutable facts, the soled ground truth that all other inference depends on.
+//! Theoretically this could all be enumerated ahead of time, but we want to be lazy and only
+//! look up things we need to. The compromise is to cache the results as they are computed.
+//!
+//! The structs in this module impl that cache in all the gory details
+
 use std::cmp::Ordering;
 use std::collections::{BTreeSet, HashMap, HashSet};
 use std::rc::Rc;
@@ -8,7 +19,7 @@ use crate::core::interning::InternedString;
 use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary};
 use crate::util::errors::CargoResult;
 
-use crate::core::resolver::types::{Candidate, ConflictReason, DepInfo};
+use crate::core::resolver::types::{Candidate, ConflictReason, DepInfo, FeaturesSet};
 use crate::core::resolver::{ActivateResult, Method};
 
 pub struct RegistryQueryer<'a> {
@@ -193,12 +204,18 @@ impl<'a> DepsCache<'a> {
         }
     }
 
+    /// Find out what dependencies will be added by activating `candidate`,
+    /// with features described in `method`. Then look up in the `registry`
+    /// the candidates that will fulfil each of these dependencies, as it is the
+    /// next obvious question.
     pub fn build_deps(
         &mut self,
         parent: Option<PackageId>,
         candidate: &Summary,
         method: &Method,
     ) -> ActivateResult<Rc<(HashSet<InternedString>, Rc<Vec<DepInfo>>)>> {
+        // if we have calculated a result before, then we can just return it,
+        // as it is a "pure" query of its arguments.
         if let Some(out) = self
             .cache
             .get(&(parent, candidate.clone(), method.clone()))
@@ -217,7 +234,7 @@ impl<'a> DepsCache<'a> {
             .into_iter()
             .map(|(dep, features)| {
                 let candidates = self.registry.query(&dep)?;
-                Ok((dep, candidates, Rc::new(features)))
+                Ok((dep, candidates, features))
             })
             .collect::<CargoResult<Vec<DepInfo>>>()?;
 
@@ -229,6 +246,8 @@ impl<'a> DepsCache<'a> {
 
         let out = Rc::new((used_features, Rc::new(deps)));
 
+        // If we succeed we add the result to the cache so we can use it again next time.
+        // We dont cache the failure cases as they dont impl Clone.
         self.cache
             .insert((parent, candidate.clone(), method.clone()), out.clone());
 
@@ -236,15 +255,13 @@ impl<'a> DepsCache<'a> {
     }
 }
 
-/// Returns all dependencies and the features we want from them.
+/// Returns the features we ended up using and
+/// all dependencies and the features we want from each of them.
 pub fn resolve_features<'b>(
     parent: Option<PackageId>,
     s: &'b Summary,
     method: &'b Method,
-) -> ActivateResult<(
-    HashSet<InternedString>,
-    Vec<(Dependency, BTreeSet<InternedString>)>,
-)> {
+) -> ActivateResult<(HashSet<InternedString>, Vec<(Dependency, FeaturesSet)>)> {
     let dev_deps = match *method {
         Method::Everything => true,
         Method::Required { dev_deps, .. } => dev_deps,
@@ -303,7 +320,7 @@ pub fn resolve_features<'b>(
                 .into());
             }
         }
-        ret.push((dep.clone(), base));
+        ret.push((dep.clone(), Rc::new(base)));
     }
 
     // Any entries in `reqs.dep` which weren't used are bugs in that the

src/cargo/core/resolver/mod.rs

@@ -47,14 +47,13 @@
 //! that we're implementing something that probably shouldn't be allocating all
 //! over the place.
 
-use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::mem;
 use std::rc::Rc;
 use std::time::{Duration, Instant};
 
 use log::{debug, trace};
 
-use crate::core::interning::InternedString;
 use crate::core::PackageIdSpec;
 use crate::core::{Dependency, PackageId, Registry, Summary};
 use crate::util::config::Config;
@@ -64,7 +63,7 @@ use crate::util::profile;
 use self::context::{Activations, Context};
 use self::dep_cache::{DepsCache, RegistryQueryer};
 use self::types::{Candidate, ConflictMap, ConflictReason, DepsFrame};
-use self::types::{RcVecIter, RemainingDeps, ResolverProgress};
+use self::types::{FeaturesSet, RcVecIter, RemainingDeps, ResolverProgress};
 
 pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use self::encode::{Metadata, WorkspaceResolve};
@@ -712,7 +711,7 @@ struct BacktrackFrame {
     remaining_candidates: RemainingCandidates,
     parent: Summary,
     dep: Dependency,
-    features: Rc<BTreeSet<InternedString>>,
+    features: FeaturesSet,
     conflicting_activations: ConflictMap,
 }
 

src/cargo/core/resolver/types.rs

@@ -89,12 +89,22 @@ impl ResolverProgress {
     }
 }
 
+/// The preferred way to store the set of activated features for a package.
+/// This is sorted so that it impls Hash, and owns its contents,
+/// needed so it can be part of the key for caching in the `DepsCache`.
+/// It is also cloned often as part of `Context`, hence the `RC`.
+/// `im-rs::OrdSet` was slower of small sets like this,
+/// but this can change with improvements to std, im, or llvm.
+/// Using a consistent type for this allows us to use the highly
+/// optimized comparison operators like `is_subset` at the interfaces.
+pub type FeaturesSet = Rc<BTreeSet<InternedString>>;
+
 #[derive(Clone, Eq, PartialEq, Hash)]
 pub enum Method {
     Everything, // equivalent to Required { dev_deps: true, all_features: true, .. }
     Required {
         dev_deps: bool,
-        features: Rc<BTreeSet<InternedString>>,
+        features: FeaturesSet,
         all_features: bool,
         uses_default_features: bool,
     },
@@ -221,7 +231,7 @@ impl RemainingDeps {
 /// Information about the dependencies for a crate, a tuple of:
 ///
 /// (dependency info, candidates, features activated)
-pub type DepInfo = (Dependency, Rc<Vec<Candidate>>, Rc<BTreeSet<InternedString>>);
+pub type DepInfo = (Dependency, Rc<Vec<Candidate>>, FeaturesSet);
 
 /// All possible reasons that a package might fail to activate.
 ///