Eliminate unneeded clones in find_or_create

...and use it to eliminate duplicate hash lookups and string copies in shape_text.

3 files changed, 32 insertions, 39 deletions

diff --git a/components/gfx/font.rs b/components/gfx/font.rs
index 6011112219b..eda41c365fe 100644
--- a/components/gfx/font.rs
+++ b/components/gfx/font.rs
@@ -166,30 +166,24 @@ impl Font {
         let shaper = &self.shaper;
         let lookup_key = ShapeCacheEntry {
             text: text.to_owned(),
-            options: options.clone(),
+            options: *options,
         };
-        if let Some(glyphs) = self.shape_cache.find(&lookup_key) {
-            return glyphs.clone();
-        }
-
-        let start_time = time::precise_time_ns();
+        self.shape_cache.find_or_create(lookup_key, || {
+            let start_time = time::precise_time_ns();
 
-        let mut glyphs = GlyphStore::new(text.len(),
-                                         options.flags.contains(IS_WHITESPACE_SHAPING_FLAG),
-                                         options.flags.contains(RTL_FLAG));
-        shaper.as_ref().unwrap().shape_text(text, options, &mut glyphs);
+            let mut glyphs = GlyphStore::new(text.len(),
+                                             options.flags.contains(IS_WHITESPACE_SHAPING_FLAG),
+                                             options.flags.contains(RTL_FLAG));
+            shaper.as_ref().unwrap().shape_text(text, options, &mut glyphs);
 
-        let glyphs = Arc::new(glyphs);
-        self.shape_cache.insert(ShapeCacheEntry {
-            text: text.to_owned(),
-            options: *options,
-        }, glyphs.clone());
+            let glyphs = Arc::new(glyphs);
 
-        let end_time = time::precise_time_ns();
-        TEXT_SHAPING_PERFORMANCE_COUNTER.fetch_add((end_time - start_time) as usize,
-                                                   Ordering::Relaxed);
+            let end_time = time::precise_time_ns();
+            TEXT_SHAPING_PERFORMANCE_COUNTER.fetch_add((end_time - start_time) as usize,
+                                                       Ordering::Relaxed);
 
-        glyphs
+            glyphs
+        })
     }
 
     fn make_shaper<'a>(&'a mut self, options: &ShapingOptions) -> &'a Shaper {
@@ -231,8 +225,8 @@ impl Font {
 
     pub fn glyph_h_advance(&mut self, glyph: GlyphId) -> FractionalPixel {
         let handle = &self.handle;
-        self.glyph_advance_cache.find_or_create(&glyph, |glyph| {
-            match handle.glyph_h_advance(*glyph) {
+        self.glyph_advance_cache.find_or_create(glyph, || {
+            match handle.glyph_h_advance(glyph) {
                 Some(adv) => adv,
                 None => 10f64 as FractionalPixel // FIXME: Need fallback strategy
             }
diff --git a/components/util/cache.rs b/components/util/cache.rs
index 7b8d7bfb1a0..7af590b015e 100644
--- a/components/util/cache.rs
+++ b/components/util/cache.rs
@@ -13,14 +13,14 @@ use std::slice::Iter;
 
 #[derive(Debug)]
 pub struct HashCache<K, V>
-    where K: Clone + PartialEq + Eq + Hash,
+    where K: PartialEq + Eq + Hash,
           V: Clone,
 {
     entries: HashMap<K, V, BuildHasherDefault<SipHasher>>,
 }
 
 impl<K, V> HashCache<K, V>
-    where K: Clone + PartialEq + Eq + Hash,
+    where K: PartialEq + Eq + Hash,
           V: Clone,
 {
     pub fn new() -> HashCache<K, V> {
@@ -40,13 +40,13 @@ impl<K, V> HashCache<K, V>
         }
     }
 
-    pub fn find_or_create<F>(&mut self, key: &K, blk: F) -> V where F: Fn(&K) -> V {
-        match self.entries.entry(key.clone()) {
+    pub fn find_or_create<F>(&mut self, key: K, mut blk: F) -> V where F: FnMut() -> V {
+        match self.entries.entry(key) {
             Occupied(occupied) => {
                 (*occupied.get()).clone()
             }
             Vacant(vacant) => {
-                (*vacant.insert(blk(key))).clone()
+                (*vacant.insert(blk())).clone()
             }
         }
     }
@@ -61,7 +61,7 @@ pub struct LRUCache<K, V> {
     cache_size: usize,
 }
 
-impl<K: Clone + PartialEq, V: Clone> LRUCache<K, V> {
+impl<K: PartialEq, V: Clone> LRUCache<K, V> {
     pub fn new(size: usize) -> LRUCache<K, V> {
         LRUCache {
           entries: vec!(),
@@ -97,12 +97,12 @@ impl<K: Clone + PartialEq, V: Clone> LRUCache<K, V> {
         }
     }
 
-    pub fn find_or_create<F>(&mut self, key: &K, blk: F) -> V where F: Fn(&K) -> V {
-        match self.entries.iter().position(|&(ref k, _)| *k == *key) {
+    pub fn find_or_create<F>(&mut self, key: K, mut blk: F) -> V where F: FnMut() -> V {
+        match self.entries.iter().position(|&(ref k, _)| *k == key) {
             Some(pos) => self.touch(pos),
             None => {
-                let val = blk(key);
-                self.insert(key.clone(), val.clone());
+                let val = blk();
+                self.insert(key, val.clone());
                 val
             }
         }
@@ -154,13 +154,12 @@ impl<K: Clone + Eq + Hash, V: Clone> SimpleHashCache<K, V> {
         }
     }
 
-    pub fn find_or_create<F>(&mut self, key: &K, blk: F) -> V where F: Fn(&K) -> V {
-        match self.find(key) {
-            Some(value) => return value,
-            None => {}
+    pub fn find_or_create<F>(&mut self, key: K, mut blk: F) -> V where F: FnMut() -> V {
+        if let Some(value) = self.find(&key) {
+            return value;
         }
-        let value = blk(key);
-        self.insert((*key).clone(), value.clone());
+        let value = blk();
+        self.insert(key, value.clone());
         value
     }
 
diff --git a/tests/unit/util/cache.rs b/tests/unit/util/cache.rs
index 1c67d81f367..9bb207622bd 100644
--- a/tests/unit/util/cache.rs
+++ b/tests/unit/util/cache.rs
@@ -13,7 +13,7 @@ fn test_hashcache() {
     assert!(cache.find(&1).is_some());
     assert!(cache.find(&2).is_none());
 
-    cache.find_or_create(&2, |_v| { Cell::new("two") });
+    cache.find_or_create(2, || { Cell::new("two") });
     assert!(cache.find(&1).is_some());
     assert!(cache.find(&2).is_some());
 }
@@ -44,7 +44,7 @@ fn test_lru_cache() {
     assert!(cache.find(&4).is_some());  // (2, 4) (no change)
 
     // Test find_or_create.
-    cache.find_or_create(&1, |_| { Cell::new("one") }); // (4, 1)
+    cache.find_or_create(1, || { Cell::new("one") }); // (4, 1)
 
     assert!(cache.find(&1).is_some()); // (4, 1) (no change)
     assert!(cache.find(&2).is_none()); // (4, 1) (no change)