Stop thrashing malloc when unioning ItemSets in UsedTemplateParameters

Previously, we were cloning an ItemSet, which requires a malloc for non-empty
sets, when taking its union with our current id's set. Now, instead of doing
that, we wrap each ItemSet in an Option, and take the set out of the hash map
when modifying it. This allows us to side-step the borrow checker and HashMap's
lack of an analog to `slice::split_at_mut` and mutate what is logically a value
in the hash map while also using immutable references of values that are
physically in the hash map.

Author: fitzgen

src/ir/named.rs

@@ -257,7 +257,7 @@ pub struct UsedTemplateParameters<'ctx, 'gen>
     where 'gen: 'ctx
 {
     ctx: &'ctx BindgenContext<'gen>,
-    used: HashMap<ItemId, ItemSet>,
+    used: HashMap<ItemId, Option<ItemSet>>,
     dependencies: HashMap<ItemId, Vec<ItemId>>,
 }
 
@@ -315,7 +315,7 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen>  {
 
         for item in ctx.whitelisted_items() {
             dependencies.entry(item).or_insert(vec![]);
-            used.insert(item, ItemSet::new());
+            used.insert(item, Some(ItemSet::new()));
 
             {
                 // We reverse our natural IR graph edges to find dependencies
@@ -359,33 +359,47 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen>  {
     }
 
     fn constrain(&mut self, id: ItemId) -> bool {
-        let original_len = self.used[&id].len();
+        // Invariant: all hash map entries' values are `Some` upon entering and
+        // exiting this method.
+        debug_assert!(self.used.values().all(|v| v.is_some()));
+
+        // Take the set for this id out of the hash map while we mutate it based
+        // on other hash map entries. We *must* put it back into the hash map at
+        // the end of this method. This allows us to side-step HashMap's lack of
+        // an analog to slice::split_at_mut.
+        let mut used_by_this_id = self.used.get_mut(&id).unwrap().take().unwrap();
+
+        let original_len = used_by_this_id.len();
 
-        // First, add this item's self template parameter usage.
         let item = self.ctx.resolve_item(id);
         let ty_kind = item.as_type().map(|ty| ty.kind());
         match ty_kind {
+            // Named template type parameters trivially use themselves.
             Some(&TypeKind::Named) => {
-                // This is a trivial use of the template type parameter.
-                self.used.get_mut(&id).unwrap().insert(id);
+                used_by_this_id.insert(id);
             }
+
+            // A template instantiation's concrete template argument is
+            // only used if the template declaration uses the
+            // corresponding template parameter.
             Some(&TypeKind::TemplateInstantiation(ref inst)) => {
                 let decl = self.ctx.resolve_type(inst.template_definition());
                 let args = inst.template_arguments();
 
-                // A template instantiation's concrete template argument is
-                // only used if the template declaration uses the
-                // corresponding template parameter.
                 let params = decl.self_template_params(self.ctx)
                     .unwrap_or(vec![]);
                 for (arg, param) in args.iter().zip(params.iter()) {
-                    if self.used[&inst.template_definition()].contains(param) {
+                    let used_by_definition = self.used[&inst.template_definition()]
+                        .as_ref()
+                        .unwrap();
+                    if used_by_definition.contains(param) {
                         if let Some(named) = arg.as_named(self.ctx, &()) {
-                            self.used.get_mut(&id).unwrap().insert(named);
+                            used_by_this_id.insert(named);
                         }
                     }
                 }
             }
+
             // Otherwise, add the union of each of its referent item's template
             // parameter usage.
             _ => {
@@ -394,20 +408,23 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen>  {
                         return;
                     }
 
-                    // This clone is unfortunate because we are potentially thrashing
-                    // malloc. We could investigate replacing the ItemSet values with
-                    // Rc<RefCell<ItemSet>> to make the borrow checker happy, but it
-                    // isn't clear that the added indirection wouldn't outweigh the cost
-                    // of malloc'ing a new ItemSet here. Ideally, `HashMap` would have a
-                    // `split_entries` method analogous to `slice::split_at_mut`...
-                    let to_add = self.used[&sub_id].clone();
-                    self.used.get_mut(&id).unwrap().extend(to_add);
+                    let used_by_sub_id = self.used[&sub_id]
+                        .as_ref()
+                        .unwrap()
+                        .iter()
+                        .cloned();
+                    used_by_this_id.extend(used_by_sub_id);
                 }, &());
             }
         }
 
-        let new_len = self.used[&id].len();
+        let new_len = used_by_this_id.len();
         assert!(new_len >= original_len);
+
+        // Put the set back in the hash map and restore our invariant.
+        self.used.insert(id, Some(used_by_this_id));
+        debug_assert!(self.used.values().all(|v| v.is_some()));
+
         new_len != original_len
     }
 
@@ -425,6 +442,9 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen>  {
 impl<'ctx, 'gen> From<UsedTemplateParameters<'ctx, 'gen>> for HashMap<ItemId, ItemSet> {
     fn from(used_templ_params: UsedTemplateParameters<'ctx, 'gen>) -> Self {
         used_templ_params.used
+            .into_iter()
+            .map(|(k, v)| (k, v.unwrap()))
+            .collect()
     }
 }