Only hash strings once in id_from_canonical

Use HashMap.entry() instead of a lookup + insert.

2 files changed, 22 insertions, 17 deletions

diff --git a/src/graph.rs b/src/graph.rs
index 03ce4b3..f15c35a 100644
--- a/src/graph.rs
+++ b/src/graph.rs
@@ -4,7 +4,7 @@ use crate::{
     densemap::{self, DenseMap},
     hash::BuildHash,
 };
-use std::collections::HashMap;
+use std::collections::{hash_map::Entry, HashMap};
 use std::path::{Path, PathBuf};
 use std::time::SystemTime;
 
@@ -310,21 +310,26 @@ impl GraphFiles {
     }
 
     /// Look up a file by its name, adding it if not already present.
-    /// Name must have been canonicalized already.
-    /// This function has a funny API to avoid copies; pass a String if you have
-    /// one already, otherwise a &str is acceptable.
-    pub fn id_from_canonical<S: AsRef<str> + Into<String>>(&mut self, file: S) -> FileId {
-        self.lookup(file.as_ref()).unwrap_or_else(|| {
-            // TODO: so many string copies :<
-            let file = file.into();
-            let id = self.by_id.push(File {
-                name: file.clone(),
-                input: None,
-                dependents: Vec::new(),
-            });
-            self.by_name.insert(file, id);
-            id
-        })
+    /// Name must have been canonicalized already. Only accepting an owned
+    /// string allows us to avoid a string copy and a hashmap lookup when we
+    /// need to create a new id, but would also be possible to create a version
+    /// of this function that accepts string references that is more optimized
+    /// for the case where the entry already exists. But so far, all of our
+    /// usages of this function have an owned string easily accessible anyways.
+    pub fn id_from_canonical(&mut self, file: String) -> FileId {
+        // TODO: so many string copies :<
+        match self.by_name.entry(file) {
+            Entry::Occupied(o) => *o.get(),
+            Entry::Vacant(v) => {
+                let id = self.by_id.push(File {
+                    name: v.key().clone(),
+                    input: None,
+                    dependents: Vec::new(),
+                });
+                v.insert(id);
+                id
+            }
+        }
     }
 
     pub fn all_ids(&self) -> impl Iterator<Item = FileId> {
diff --git a/src/work.rs b/src/work.rs
index 8079ad1..6366c72 100644
--- a/src/work.rs
+++ b/src/work.rs
@@ -788,7 +788,7 @@ build b: phony c
 build c: phony a
 ";
         let mut graph = crate::load::parse("build.ninja", file.as_bytes().to_vec())?;
-        let a_id = graph.files.id_from_canonical("a");
+        let a_id = graph.files.id_from_canonical("a".to_owned());
         let mut states = BuildStates::new(graph.builds.next_id(), SmallMap::default());
         let mut stack = Vec::new();
         match states.want_file(&graph, &mut stack, a_id) {