servo: Merge #11180 - Minor cleanup and optimizations in glyph/style caching (from mbrubeck:text-cleanup); r=pcwalton
authorMatt Brubeck <mbrubeck@limpet.net>
Sat, 14 May 2016 09:06:53 -0700
changeset 338804 327d24febd2e8995659dba7550b1ef41d41f7781
parent 338803 918b7393243cda1b8c3c072e89e8d77d6ac00a25
child 338805 59cc086bc468f7cbaa1f863e9ea13acb49a11333
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspcwalton
servo: Merge #11180 - Minor cleanup and optimizations in glyph/style caching (from mbrubeck:text-cleanup); r=pcwalton Gets rid of some unnecessary String and Arc clones during text shaping and style matching. r? @pcwalton Source-Repo: https://github.com/servo/servo Source-Revision: e2990766dc1c7461b55c96f0ce7116d35d4fd3c6
servo/components/gfx/font.rs
servo/components/gfx/font_context.rs
servo/components/style/matching.rs
servo/components/util/cache.rs
servo/tests/unit/util/cache.rs
--- a/servo/components/gfx/font.rs
+++ b/servo/components/gfx/font.rs
@@ -11,17 +11,16 @@ use platform::font_template::FontTemplat
 use smallvec::SmallVec;
 use std::borrow::ToOwned;
 use std::cell::RefCell;
 use std::rc::Rc;
 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;
 use time;
 use unicode_script::Script;
 use util::cache::HashCache;
 use webrender_traits;
 
@@ -49,17 +48,17 @@ pub trait FontHandleMethods: Sized {
     fn table_for_tag(&self, FontTableTag) -> Option<Box<FontTable>>;
 }
 
 // Used to abstract over the shaper's choice of fixed int representation.
 pub type FractionalPixel = f64;
 
 pub type FontTableTag = u32;
 
-pub trait FontTableTagConversions {
+trait FontTableTagConversions {
     fn tag_to_str(&self) -> String;
 }
 
 impl FontTableTagConversions for FontTableTag {
     fn tag_to_str(&self) -> String {
         let bytes = [(self >> 24) as u8,
                      (self >> 16) as u8,
                      (self >>  8) as u8,
@@ -83,32 +82,53 @@ pub struct FontMetrics {
     pub em_size:          Au,
     pub ascent:           Au,
     pub descent:          Au,
     pub max_advance:      Au,
     pub average_advance:  Au,
     pub line_gap:         Au,
 }
 
-pub type SpecifiedFontStyle = ServoFont;
-
 #[derive(Debug)]
 pub struct Font {
     pub handle: FontHandle,
     pub metrics: FontMetrics,
     pub variant: font_variant::T,
     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."]
         const IS_WHITESPACE_SHAPING_FLAG = 0x01,
         #[doc = "Set if we are to ignore ligatures."]
         const IGNORE_LIGATURES_SHAPING_FLAG = 0x02,
         #[doc = "Set if we are to disable kerning."]
         const DISABLE_KERNING_SHAPING_FLAG = 0x04,
@@ -128,53 +148,47 @@ pub struct ShapingOptions {
     /// The Unicode script property of the characters in this run.
     pub script: Script,
     /// Various flags.
     pub flags: ShapingFlags,
 }
 
 /// An entry in the shape cache.
 #[derive(Clone, Eq, PartialEq, Hash, Debug)]
-pub struct ShapeCacheEntry {
+struct ShapeCacheEntry {
     text: String,
     options: ShapingOptions,
 }
 
 impl Font {
     pub fn shape_text(&mut self, text: &str, options: &ShapingOptions) -> Arc<GlyphStore> {
         self.make_shaper(options);
 
         //FIXME: find the equivalent of Equiv and the old ShapeCacheEntryRef
         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);
 
-        let glyphs = Arc::new(glyphs);
-        self.shape_cache.insert(ShapeCacheEntry {
-            text: text.to_owned(),
-            options: *options,
-        }, glyphs.clone());
+            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 {
         // fast path: already created a shaper
         if let Some(ref mut shaper) = self.shaper {
             shaper.set_options(options);
             return shaper
         }
@@ -206,18 +220,18 @@ impl Font {
 
     pub fn glyph_h_kerning(&mut self, first_glyph: GlyphId, second_glyph: GlyphId)
                            -> FractionalPixel {
         self.handle.glyph_h_kerning(first_glyph, second_glyph)
     }
 
     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
             }
         })
     }
 }
 
 #[derive(Debug)]
--- a/servo/components/gfx/font_context.rs
+++ b/servo/components/gfx/font_context.rs
@@ -3,36 +3,34 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use app_units::Au;
 use azure::azure_hl::BackendType;
 #[cfg(any(target_os = "linux", target_os = "android", target_os = "windows"))]
 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;
 use platform::font::FontHandle;
 use platform::font_context::FontContextHandle;
 use platform::font_template::FontTemplateData;
 use smallvec::SmallVec;
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::default::Default;
 use std::hash::{BuildHasherDefault, Hash, Hasher};
 use std::rc::Rc;
 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"))]
 fn create_scaled_font(template: &Arc<FontTemplateData>, pt_size: Au) -> ScaledFont {
     ScaledFont::new(BackendType::Skia, FontInfo::FontData(&template.bytes),
                     pt_size.to_f32_px())
 }
 
@@ -119,32 +117,18 @@ impl FontContext {
             font_variant::T::small_caps => pt_size.scale_by(SMALL_CAPS_SCALE_FACTOR),
             font_variant::T::normal => pt_size,
         };
 
         let handle: Result<FontHandle, _> =
             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) {
         let current_epoch = FONT_CACHE_EPOCH.load(Ordering::SeqCst);
         if current_epoch == self.epoch {
             return
         }
 
@@ -153,17 +137,17 @@ impl FontContext {
         self.paint_font_cache.clear();
         self.layout_font_group_cache.clear();
         self.epoch = current_epoch
     }
 
     /// 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();
 
         let layout_font_group_cache_key = LayoutFontGroupCacheKey {
             pointer: style.clone(),
             size: style.font_size,
         };
         if let Some(ref cached_font_group) = self.layout_font_group_cache.get(
@@ -312,17 +296,17 @@ impl HeapSizeOf for FontContext {
     fn heap_size_of_children(&self) -> usize {
         // FIXME(njn): Measure other fields eventually.
         self.platform_handle.heap_size_of_children()
     }
 }
 
 #[derive(Debug)]
 struct LayoutFontGroupCacheKey {
-    pointer: Arc<SpecifiedFontStyle>,
+    pointer: Arc<ServoFont>,
     size: Au,
 }
 
 impl PartialEq for LayoutFontGroupCacheKey {
     fn eq(&self, other: &LayoutFontGroupCacheKey) -> bool {
         self.pointer == other.pointer && self.size == other.size
     }
 }
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -346,17 +346,17 @@ impl<C: ComputedValues> StyleSharingCand
     pub fn insert_if_possible<N: TNode<ConcreteComputedValues=C>>(&mut self, element: &N::ConcreteElement) {
         match StyleSharingCandidate::new::<N>(element) {
             None => {}
             Some(candidate) => self.cache.insert(candidate, ())
         }
     }
 
     pub fn touch(&mut self, index: usize) {
-        self.cache.touch(index)
+        self.cache.touch(index);
     }
 }
 
 /// The results of attempting to share a style.
 pub enum StyleSharingResult<ConcreteRestyleDamage: TRestyleDamage> {
     /// We didn't find anybody to share the style with.
     CannotShare,
     /// The node's style can be shared. The integer specifies the index in the LRU cache that was
--- a/servo/components/util/cache.rs
+++ b/servo/components/util/cache.rs
@@ -8,24 +8,24 @@ use std::collections::HashMap;
 use std::collections::hash_map::Entry::{Occupied, Vacant};
 use std::default::Default;
 use std::hash::{BuildHasherDefault, Hash, Hasher, SipHasher};
 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> {
         HashCache {
           entries: HashMap::with_hasher(Default::default()),
         }
     }
 
@@ -35,79 +35,79 @@ impl<K, V> HashCache<K, V>
 
     pub fn find(&self, key: &K) -> Option<V> {
         match self.entries.get(key) {
             Some(v) => Some(v.clone()),
             None => None,
         }
     }
 
-    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()
             }
         }
     }
 
     pub fn evict_all(&mut self) {
         self.entries.clear();
     }
 }
 
 pub struct LRUCache<K, V> {
     entries: Vec<(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!(),
           cache_size: size,
         }
     }
 
     #[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)> {
         self.entries.iter()
     }
 
     pub fn insert(&mut self, key: K, val: V) {
         if self.entries.len() == self.cache_size {
             self.entries.remove(0);
         }
         self.entries.push((key, val));
     }
 
     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
             }
         }
     }
 
     pub fn evict_all(&mut self) {
         self.entries.clear();
     }
@@ -149,23 +149,22 @@ impl<K: Clone + Eq + Hash, V: Clone> Sim
     pub fn find<Q>(&self, key: &Q) -> Option<V> where Q: PartialEq<K> + Hash + Eq {
         let bucket_index = self.bucket_for_key(key);
         match self.entries[bucket_index] {
             Some((ref existing_key, ref value)) if key == existing_key => Some((*value).clone()),
             _ => None,
         }
     }
 
-    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
     }
 
     pub fn evict_all(&mut self) {
         for slot in &mut self.entries {
             *slot = None
         }
     }
--- a/servo/tests/unit/util/cache.rs
+++ b/servo/tests/unit/util/cache.rs
@@ -8,17 +8,17 @@ use util::cache::{HashCache, LRUCache};
 #[test]
 fn test_hashcache() {
     let mut cache: HashCache<usize, Cell<&str>> = HashCache::new();
 
     cache.insert(1, Cell::new("one"));
     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());
 }
 
 #[test]
 fn test_lru_cache() {
     let one = Cell::new("one");
     let two = Cell::new("two");
@@ -39,15 +39,15 @@ fn test_lru_cache() {
     cache.insert(4, four); // (2, 4)
 
     assert!(cache.find(&1).is_none());  // (2, 4) (no change)
     assert!(cache.find(&2).is_some());  // (4, 2)
     assert!(cache.find(&3).is_none());  // (4, 2) (no change)
     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)
     assert!(cache.find(&3).is_none()); // (4, 1) (no change)
     assert!(cache.find(&4).is_some()); // (1, 4)
 }