Auto merge of #11347 - mbrubeck:fast-shape, r=pcwalton

Don't create HarfBuzz shaper if it isn't used

Move the fast shaping code out of `text::shaping::harfbuzz`, and initialize the HarfBuzz shaper lazily to avoid allocating unnecessary HarfBuzz objects.

Note: As the fast shaping code grows and gains OpenType support, I'll probably factor it out into a whole new `text::shaping::fast` module.

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/11347)
<!-- Reviewable:end -->

2 files changed, 71 insertions, 106 deletions

diff --git a/components/gfx/font.rs b/components/gfx/font.rs
index 7fdf217d18e..c401c616d8f 100644
--- a/components/gfx/font.rs
+++ b/components/gfx/font.rs
@@ -9,6 +9,7 @@ use platform::font::{FontHandle, FontTable};
 use platform::font_context::FontContextHandle;
 use platform::font_template::FontTemplateData;
 use smallvec::SmallVec;
+use std::ascii::AsciiExt;
 use std::borrow::ToOwned;
 use std::cell::RefCell;
 use std::rc::Rc;
@@ -17,7 +18,7 @@ use std::sync::Arc;
 use std::sync::atomic::{ATOMIC_USIZE_INIT, AtomicUsize, Ordering};
 use style::computed_values::{font_stretch, font_variant, font_weight};
 use text::Shaper;
-use text::glyph::{GlyphId, GlyphStore};
+use text::glyph::{ByteIndex, GlyphData, GlyphId, GlyphStore};
 use text::shaping::ShaperMethods;
 use time;
 use unicode_script::Script;
@@ -108,8 +109,8 @@ pub struct Font {
     pub requested_pt_size: Au,
     pub actual_pt_size: Au,
     shaper: Option<Shaper>,
-    shape_cache: HashCache<ShapeCacheEntry, Arc<GlyphStore>>,
-    glyph_advance_cache: HashCache<u32, FractionalPixel>,
+    shape_cache: RefCell<HashCache<ShapeCacheEntry, Arc<GlyphStore>>>,
+    glyph_advance_cache: RefCell<HashCache<u32, FractionalPixel>>,
     pub font_key: Option<webrender_traits::FontKey>,
 }
 
@@ -129,8 +130,8 @@ impl Font {
             requested_pt_size: requested_pt_size,
             actual_pt_size: actual_pt_size,
             metrics: metrics,
-            shape_cache: HashCache::new(),
-            glyph_advance_cache: HashCache::new(),
+            shape_cache: RefCell::new(HashCache::new()),
+            glyph_advance_cache: RefCell::new(HashCache::new()),
             font_key: font_key,
         }
     }
@@ -172,42 +173,74 @@ struct ShapeCacheEntry {
 
 impl Font {
     pub fn shape_text(&mut self, text: &str, options: &ShapingOptions) -> Arc<GlyphStore> {
-        self.make_shaper(options);
+        let this = self as *const Font;
+        let mut shaper = self.shaper.take();
 
-        //FIXME: find the equivalent of Equiv and the old ShapeCacheEntryRef
-        let shaper = &self.shaper;
         let lookup_key = ShapeCacheEntry {
             text: text.to_owned(),
             options: *options,
         };
-        self.shape_cache.find_or_create(lookup_key, || {
+        let result = self.shape_cache.borrow_mut().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 glyphs = Arc::new(glyphs);
+            if self.can_do_fast_shaping(text, options) {
+                debug!("shape_text: Using ASCII fast path.");
+                self.shape_text_fast(text, options, &mut glyphs);
+            } else {
+                debug!("shape_text: Using Harfbuzz.");
+                if shaper.is_none() {
+                    shaper = Some(Shaper::new(this));
+                }
+                shaper.as_ref().unwrap().shape_text(text, options, &mut glyphs);
+            }
 
             let end_time = time::precise_time_ns();
             TEXT_SHAPING_PERFORMANCE_COUNTER.fetch_add((end_time - start_time) as usize,
                                                        Ordering::Relaxed);
+            Arc::new(glyphs)
+        });
+        self.shaper = shaper;
+        result
+    }
 
-            glyphs
-        })
+    fn can_do_fast_shaping(&self, text: &str, options: &ShapingOptions) -> bool {
+        options.script == Script::Latin &&
+            !options.flags.contains(RTL_FLAG) &&
+            self.handle.can_do_fast_shaping() &&
+            text.is_ascii()
     }
 
-    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
+    /// Fast path for ASCII text that only needs simple horizontal LTR kerning.
+    fn shape_text_fast(&self, text: &str, options: &ShapingOptions, glyphs: &mut GlyphStore) {
+        let mut prev_glyph_id = None;
+        for (i, byte) in text.bytes().enumerate() {
+            let character = byte as char;
+            let glyph_id = match self.glyph_index(character) {
+                Some(id) => id,
+                None => continue,
+            };
+
+            let mut advance = Au::from_f64_px(self.glyph_h_advance(glyph_id));
+            if character == ' ' {
+                advance += options.word_spacing;
+            }
+            if let Some(letter_spacing) = options.letter_spacing {
+                advance += letter_spacing;
+            }
+            let offset = prev_glyph_id.map(|prev| {
+                let h_kerning = Au::from_f64_px(self.glyph_h_kerning(prev, glyph_id));
+                advance += h_kerning;
+                Point2D::new(h_kerning, Au(0))
+            });
+
+            let glyph = GlyphData::new(glyph_id, advance, offset, true, true);
+            glyphs.add_glyph_for_byte_index(ByteIndex(i as isize), character, &glyph);
+            prev_glyph_id = Some(glyph_id);
         }
-
-        let shaper = Shaper::new(self, options);
-        self.shaper = Some(shaper);
-        self.shaper.as_ref().unwrap()
+        glyphs.finalize_changes();
     }
 
     pub fn table_for_tag(&self, tag: FontTableTag) -> Option<FontTable> {
@@ -230,15 +263,14 @@ impl Font {
         self.handle.glyph_index(codepoint)
     }
 
-    pub fn glyph_h_kerning(&mut self, first_glyph: GlyphId, second_glyph: GlyphId)
+    pub fn glyph_h_kerning(&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, || {
-            match handle.glyph_h_advance(glyph) {
+    pub fn glyph_h_advance(&self, glyph: GlyphId) -> FractionalPixel {
+        self.glyph_advance_cache.borrow_mut().find_or_create(glyph, || {
+            match self.handle.glyph_h_advance(glyph) {
                 Some(adv) => adv,
                 None => 10f64 as FractionalPixel // FIXME: Need fallback strategy
             }
diff --git a/components/gfx/text/shaping/harfbuzz.rs b/components/gfx/text/shaping/harfbuzz.rs
index 79d36f9b6e0..517e20917c5 100644
--- a/components/gfx/text/shaping/harfbuzz.rs
+++ b/components/gfx/text/shaping/harfbuzz.rs
@@ -4,7 +4,7 @@
 
 use app_units::Au;
 use euclid::Point2D;
-use font::{DISABLE_KERNING_SHAPING_FLAG, Font, FontHandleMethods, FontTableMethods, FontTableTag};
+use font::{DISABLE_KERNING_SHAPING_FLAG, Font, FontTableMethods, FontTableTag};
 use font::{IGNORE_LIGATURES_SHAPING_FLAG, KERN, RTL_FLAG, ShapingOptions};
 use harfbuzz::{HB_DIRECTION_LTR, HB_DIRECTION_RTL, HB_MEMORY_MODE_READONLY};
 use harfbuzz::{hb_blob_create, hb_face_create_for_tables};
@@ -34,12 +34,10 @@ use harfbuzz::{hb_glyph_position_t};
 use harfbuzz::{hb_position_t, hb_tag_t};
 use libc::{c_char, c_int, c_uint, c_void};
 use platform::font::FontTable;
-use std::ascii::AsciiExt;
 use std::{char, cmp, ptr};
 use text::glyph::{ByteIndex, GlyphData, GlyphId, GlyphStore};
 use text::shaping::ShaperMethods;
 use text::util::{fixed_to_float, float_to_fixed, is_bidi_control};
-use unicode_script::Script;
 
 const NO_GLYPH: i32 = -1;
 const LIGA: u32 = ot_tag!('l', 'i', 'g', 'a');
@@ -127,16 +125,10 @@ impl ShapedGlyphData {
 }
 
 #[derive(Debug)]
-struct FontAndShapingOptions {
-    font: *mut Font,
-    options: ShapingOptions,
-}
-
-#[derive(Debug)]
 pub struct Shaper {
     hb_face: *mut hb_face_t,
     hb_font: *mut hb_font_t,
-    font_and_shaping_options: Box<FontAndShapingOptions>,
+    font: *const Font,
 }
 
 impl Drop for Shaper {
@@ -152,20 +144,16 @@ impl Drop for Shaper {
 }
 
 impl Shaper {
-    pub fn new(font: &mut Font, options: &ShapingOptions) -> Shaper {
+    pub fn new(font: *const Font) -> Shaper {
         unsafe {
-            let mut font_and_shaping_options = box FontAndShapingOptions {
-                font: font,
-                options: *options,
-            };
             let hb_face: *mut hb_face_t =
                 hb_face_create_for_tables(Some(font_table_func),
-                                          &mut *font_and_shaping_options as *mut _ as *mut c_void,
+                                          font as *const c_void as *mut c_void,
                                           None);
             let hb_font: *mut hb_font_t = hb_font_create(hb_face);
 
             // Set points-per-em. if zero, performs no hinting in that direction.
-            let pt_size = font.actual_pt_size.to_f64_px();
+            let pt_size = (*font).actual_pt_size.to_f64_px();
             hb_font_set_ppem(hb_font, pt_size as c_uint, pt_size as c_uint);
 
             // Set scaling. Note that this takes 16.16 fixed point.
@@ -179,15 +167,11 @@ impl Shaper {
             Shaper {
                 hb_face: hb_face,
                 hb_font: hb_font,
-                font_and_shaping_options: font_and_shaping_options,
+                font: font,
             }
         }
     }
 
-    pub fn set_options(&mut self, options: &ShapingOptions) {
-        self.font_and_shaping_options.options = *options
-    }
-
     fn float_to_fixed(f: f64) -> i32 {
         float_to_fixed(16, f)
     }
@@ -201,14 +185,7 @@ impl ShaperMethods for Shaper {
     /// Calculate the layout metrics associated with the given text when painted in a specific
     /// font.
     fn shape_text(&self, text: &str, options: &ShapingOptions, glyphs: &mut GlyphStore) {
-        if self.can_use_fast_path(text, options) {
-            debug!("shape_text: Using ASCII fast path.");
-            self.shape_text_fast(text, options, glyphs);
-            return;
-        }
         unsafe {
-            debug!("shape_text: Using Harfbuzz.");
-
             let hb_buffer: *mut hb_buffer_t = hb_buffer_create();
             hb_buffer_set_direction(hb_buffer, if options.flags.contains(RTL_FLAG) {
                 HB_DIRECTION_RTL
@@ -250,47 +227,6 @@ impl ShaperMethods for Shaper {
 }
 
 impl Shaper {
-    fn can_use_fast_path(&self, text: &str, options: &ShapingOptions) -> bool {
-        let font = self.font_and_shaping_options.font;
-
-        options.script == Script::Latin &&
-            !options.flags.contains(RTL_FLAG) &&
-            unsafe { (*font).handle.can_do_fast_shaping() } &&
-            text.is_ascii()
-    }
-
-    /// Fast path for ASCII text that only needs simple horizontal LTR kerning.
-    fn shape_text_fast(&self, text: &str, options: &ShapingOptions, glyphs: &mut GlyphStore) {
-        let font = unsafe { &mut *self.font_and_shaping_options.font };
-
-        let mut prev_glyph_id = None;
-        for (i, byte) in text.bytes().enumerate() {
-            let character = byte as char;
-            let glyph_id = match font.glyph_index(character) {
-                Some(id) => id,
-                None => continue,
-            };
-
-            let mut advance = Au::from_f64_px(font.glyph_h_advance(glyph_id));
-            if character == ' ' {
-                advance += options.word_spacing;
-            }
-            if let Some(letter_spacing) = options.letter_spacing {
-                advance += letter_spacing;
-            }
-            let offset = prev_glyph_id.map(|prev| {
-                let h_kerning = Au::from_f64_px(font.glyph_h_kerning(prev, glyph_id));
-                advance += h_kerning;
-                Point2D::new(h_kerning, Au(0))
-            });
-
-            let glyph = GlyphData::new(glyph_id, advance, offset, true, true);
-            glyphs.add_glyph_for_byte_index(ByteIndex(i as isize), character, &glyph);
-            prev_glyph_id = Some(glyph_id);
-        }
-        glyphs.finalize_changes();
-    }
-
     fn save_glyph_results(&self,
                           text: &str,
                           options: &ShapingOptions,
@@ -409,8 +345,7 @@ impl Shaper {
                     //
                     // TODO: Proper tab stops.
                     const TAB_COLS: i32 = 8;
-                    let font = self.font_and_shaping_options.font;
-                    let (space_glyph_id, space_advance) = glyph_space_advance(font);
+                    let (space_glyph_id, space_advance) = glyph_space_advance(self.font);
                     let advance = Au::from_f64_px(space_advance) * TAB_COLS;
                     let data = GlyphData::new(space_glyph_id,
                                               advance,
@@ -522,7 +457,7 @@ extern fn glyph_h_advance_func(_: *mut hb_font_t,
     }
 }
 
-fn glyph_space_advance(font: *mut Font) -> (hb_codepoint_t, f64) {
+fn glyph_space_advance(font: *const Font) -> (hb_codepoint_t, f64) {
     let space_unicode = ' ';
     let space_glyph: hb_codepoint_t;
     match unsafe { (*font).glyph_index(space_unicode) } {
@@ -557,13 +492,11 @@ extern fn font_table_func(_: *mut hb_face_t,
                               -> *mut hb_blob_t {
     unsafe {
         // NB: These asserts have security implications.
-        let font_and_shaping_options: *const FontAndShapingOptions =
-            user_data as *const FontAndShapingOptions;
-        assert!(!font_and_shaping_options.is_null());
-        assert!(!(*font_and_shaping_options).font.is_null());
+        let font = user_data as *const Font;
+        assert!(!font.is_null());
 
         // TODO(Issue #197): reuse font table data, which will change the unsound trickery here.
-        match (*(*font_and_shaping_options).font).table_for_tag(tag as FontTableTag) {
+        match (*font).table_for_tag(tag as FontTableTag) {
             None => ptr::null_mut(),
             Some(font_table) => {
                 // `Box::into_raw` intentionally leaks the FontTable so we don't destroy the buffer