Auto merge of #11180 - mbrubeck:text-cleanup, r=pcwalton

Minor cleanup and optimizations in glyph/style caching

Gets rid of some unnecessary String and Arc clones during text shaping and style matching.

r? @pcwalton

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11180)
<!-- Reviewable:end -->

5 files changed, 71 insertions, 74 deletions

diff --git a/components/gfx/font.rs b/components/gfx/font.rs
index a6b2af9c8a2..eda41c365fe 100644
--- a/components/gfx/font.rs
+++ b/components/gfx/font.rs
@@ -16,7 +16,6 @@ use std::str;
 use std::sync::Arc;
 use std::sync::atomic::{ATOMIC_USIZE_INIT, AtomicUsize, Ordering};
 use style::computed_values::{font_stretch, font_variant, font_weight};
-use style::properties::style_structs::ServoFont;
 use text::Shaper;
 use text::glyph::{GlyphId, GlyphStore};
 use text::shaping::ShaperMethods;
@@ -54,7 +53,7 @@ pub type FractionalPixel = f64;
 
 pub type FontTableTag = u32;
 
-pub trait FontTableTagConversions {
+trait FontTableTagConversions {
     fn tag_to_str(&self) -> String;
 }
 
@@ -88,8 +87,6 @@ pub struct FontMetrics {
     pub line_gap:         Au,
 }
 
-pub type SpecifiedFontStyle = ServoFont;
-
 #[derive(Debug)]
 pub struct Font {
     pub handle: FontHandle,
@@ -98,12 +95,35 @@ pub struct Font {
     pub descriptor: FontTemplateDescriptor,
     pub requested_pt_size: Au,
     pub actual_pt_size: Au,
-    pub shaper: Option<Shaper>,
-    pub shape_cache: HashCache<ShapeCacheEntry, Arc<GlyphStore>>,
-    pub glyph_advance_cache: HashCache<u32, FractionalPixel>,
+    shaper: Option<Shaper>,
+    shape_cache: HashCache<ShapeCacheEntry, Arc<GlyphStore>>,
+    glyph_advance_cache: HashCache<u32, FractionalPixel>,
     pub font_key: Option<webrender_traits::FontKey>,
 }
 
+impl Font {
+    pub fn new(handle: FontHandle,
+               variant: font_variant::T,
+               descriptor: FontTemplateDescriptor,
+               requested_pt_size: Au,
+               actual_pt_size: Au,
+               font_key: Option<webrender_traits::FontKey>) -> Font {
+        let metrics = handle.metrics();
+        Font {
+            handle: handle,
+            shaper: None,
+            variant: variant,
+            descriptor: descriptor,
+            requested_pt_size: requested_pt_size,
+            actual_pt_size: actual_pt_size,
+            metrics: metrics,
+            shape_cache: HashCache::new(),
+            glyph_advance_cache: HashCache::new(),
+            font_key: font_key,
+        }
+    }
+}
+
 bitflags! {
     pub flags ShapingFlags: u8 {
         #[doc = "Set if the text is entirely whitespace."]
@@ -133,7 +153,7 @@ pub struct ShapingOptions {
 
 /// An entry in the shape cache.
 #[derive(Clone, Eq, PartialEq, Hash, Debug)]
-pub struct ShapeCacheEntry {
+struct ShapeCacheEntry {
     text: String,
     options: ShapingOptions,
 }
@@ -146,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 {
@@ -211,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/gfx/font_context.rs b/components/gfx/font_context.rs
index ff7715e2cb7..cb572909b38 100644
--- a/components/gfx/font_context.rs
+++ b/components/gfx/font_context.rs
@@ -8,9 +8,7 @@ use azure::azure_hl::BackendType;
 use azure::scaled_font::FontInfo;
 use azure::scaled_font::ScaledFont;
 use fnv::FnvHasher;
-use font::FontHandleMethods;
-use font::SpecifiedFontStyle;
-use font::{Font, FontGroup};
+use font::{Font, FontGroup, FontHandleMethods};
 use font_cache_thread::FontCacheThread;
 use font_template::FontTemplateDescriptor;
 use heapsize::HeapSizeOf;
@@ -27,7 +25,7 @@ use std::sync::Arc;
 use std::sync::atomic::{AtomicUsize, Ordering, ATOMIC_USIZE_INIT};
 use string_cache::Atom;
 use style::computed_values::{font_style, font_variant};
-use util::cache::HashCache;
+use style::properties::style_structs::ServoFont;
 use webrender_traits;
 
 #[cfg(any(target_os = "linux", target_os = "android", target_os = "windows"))]
@@ -124,22 +122,8 @@ impl FontContext {
             FontHandleMethods::new_from_template(&self.platform_handle, template,
                                                  Some(actual_pt_size));
 
-        handle.map(|handle| {
-            let metrics = handle.metrics();
-
-            Font {
-                handle: handle,
-                shaper: None,
-                variant: variant,
-                descriptor: descriptor,
-                requested_pt_size: pt_size,
-                actual_pt_size: actual_pt_size,
-                metrics: metrics,
-                shape_cache: HashCache::new(),
-                glyph_advance_cache: HashCache::new(),
-                font_key: font_key,
-            }
-        })
+        handle.map(|handle|
+            Font::new(handle, variant, descriptor, pt_size, actual_pt_size, font_key))
     }
 
     fn expire_font_caches_if_necessary(&mut self) {
@@ -158,7 +142,7 @@ impl FontContext {
     /// Create a group of fonts for use in layout calculations. May return
     /// a cached font if this font instance has already been used by
     /// this context.
-    pub fn layout_font_group_for_style(&mut self, style: Arc<SpecifiedFontStyle>)
+    pub fn layout_font_group_for_style(&mut self, style: Arc<ServoFont>)
                                        -> Rc<FontGroup> {
         self.expire_font_caches_if_necessary();
 
@@ -317,7 +301,7 @@ impl HeapSizeOf for FontContext {
 
 #[derive(Debug)]
 struct LayoutFontGroupCacheKey {
-    pointer: Arc<SpecifiedFontStyle>,
+    pointer: Arc<ServoFont>,
     size: Au,
 }
 
diff --git a/components/style/matching.rs b/components/style/matching.rs
index 11fa2915081..89db58a04db 100644
--- a/components/style/matching.rs
+++ b/components/style/matching.rs
@@ -351,7 +351,7 @@ impl<C: ComputedValues> StyleSharingCandidateCache<C> {
     }
 
     pub fn touch(&mut self, index: usize) {
-        self.cache.touch(index)
+        self.cache.touch(index);
     }
 }
 
diff --git a/components/util/cache.rs b/components/util/cache.rs
index 7b8d7bfb1a0..f446f43b498 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!(),
@@ -70,13 +70,13 @@ impl<K: Clone + PartialEq, V: Clone> LRUCache<K, V> {
     }
 
     #[inline]
-    pub fn touch(&mut self, pos: usize) -> V {
+    pub fn touch(&mut self, pos: usize) -> &V {
         let last_index = self.entries.len() - 1;
         if pos != last_index {
             let entry = self.entries.remove(pos);
             self.entries.push(entry);
         }
-        self.entries[last_index].1.clone()
+        &self.entries[last_index].1
     }
 
     pub fn iter(&self) -> Iter<(K, V)> {
@@ -92,17 +92,17 @@ impl<K: Clone + PartialEq, V: Clone> LRUCache<K, V> {
 
     pub fn find(&mut self, key: &K) -> Option<V> {
         match self.entries.iter().position(|&(ref k, _)| key == k) {
-            Some(pos) => Some(self.touch(pos)),
+            Some(pos) => Some(self.touch(pos).clone()),
             None      => None,
         }
     }
 
-    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) {
-            Some(pos) => self.touch(pos),
+    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).clone(),
             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)