Only hash strings once in id_from_canonical
Use HashMap.entry() instead of a lookup + insert.
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 @@
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 @@
}
/// 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 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) {
