From dc683a1fc928af7bd30d6420128a8d31c89067dc Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 1 Apr 2018 21:04:18 +1000 Subject: [PATCH] Linux: Don't hold onto bytes of system fonts FontTemplateData gets passed over IPC during the communication between FontContext and FontCacheThread. Serializing and deserializing these bytes is expensive, so this change ensures that we only do that when the bytes can't be read from disk. A similar strategy is already used on macos and windows. The performance problem was particularly noticeable after implenting font fallback, where the content process would potentially work through a list of fonts, trying to find one which contains a certain glyph. That could result in lots of font bytes going over IPC. --- components/gfx/platform/freetype/font.rs | 78 +++++++++++-------- .../gfx/platform/freetype/font_template.rs | 37 +++++---- 2 files changed, 65 insertions(+), 50 deletions(-) diff --git a/components/gfx/platform/freetype/font.rs b/components/gfx/platform/freetype/font.rs index 131d3deb8db..6c2a6b92696 100644 --- a/components/gfx/platform/freetype/font.rs +++ b/components/gfx/platform/freetype/font.rs @@ -5,7 +5,7 @@ use app_units::Au; use font::{FontHandleMethods, FontMetrics, FontTableMethods}; use font::{FontTableTag, FractionalPixel, GPOS, GSUB, KERN}; -use freetype::freetype::{FT_Done_Face, FT_New_Memory_Face}; +use freetype::freetype::{FT_Done_Face, FT_New_Face, FT_New_Memory_Face}; use freetype::freetype::{FT_F26Dot6, FT_Face, FT_FaceRec}; use freetype::freetype::{FT_Get_Char_Index, FT_Get_Postscript_Name}; use freetype::freetype::{FT_Get_Kerning, FT_Get_Sfnt_Table, FT_Load_Sfnt_Table}; @@ -20,6 +20,7 @@ use platform::font_context::FontContextHandle; use platform::font_template::FontTemplateData; use servo_atoms::Atom; use std::{mem, ptr}; +use std::ffi::CString; use std::os::raw::{c_char, c_long}; use std::sync::Arc; use style::computed_values::font_stretch::T as FontStretch; @@ -87,6 +88,39 @@ impl Drop for FontHandle { } } +fn create_face( + lib: FT_Library, + template: &FontTemplateData, + pt_size: Option, +) -> Result { + unsafe { + let mut face: FT_Face = ptr::null_mut(); + let face_index = 0 as FT_Long; + + let result = if let Some(ref bytes) = template.bytes { + FT_New_Memory_Face(lib, bytes.as_ptr(), bytes.len() as FT_Long, face_index, &mut face) + } else { + // This will trigger a synchronous file read in the layout thread, which we may want to + // revisit at some point. See discussion here: + // + // https://github.com/servo/servo/pull/20506#issuecomment-378838800 + + let filename = CString::new(&*template.identifier).expect("filename contains NUL byte!"); + FT_New_Face(lib, filename.as_ptr(), face_index, &mut face) + }; + + if !succeeded(result) || face.is_null() { + return Err(()); + } + + if let Some(s) = pt_size { + FontHandle::set_char_size(face, s).or(Err(()))? + } + + Ok(face) + } +} + impl FontHandleMethods for FontHandle { fn new_from_template(fctx: &FontContextHandle, template: Arc, @@ -95,37 +129,19 @@ impl FontHandleMethods for FontHandle { let ft_ctx: FT_Library = fctx.ctx.ctx; if ft_ctx.is_null() { return Err(()); } - return create_face_from_buffer(ft_ctx, &template.bytes, pt_size).map(|face| { - let mut handle = FontHandle { - face: face, - font_data: template.clone(), - handle: fctx.clone(), - can_do_fast_shaping: false, - }; - // TODO (#11310): Implement basic support for GPOS and GSUB. - handle.can_do_fast_shaping = handle.has_table(KERN) && - !handle.has_table(GPOS) && - !handle.has_table(GSUB); - handle - }); + let face = create_face(ft_ctx, &template, pt_size)?; - fn create_face_from_buffer(lib: FT_Library, buffer: &[u8], pt_size: Option) - -> Result { - unsafe { - let mut face: FT_Face = ptr::null_mut(); - let face_index = 0 as FT_Long; - let result = FT_New_Memory_Face(lib, buffer.as_ptr(), buffer.len() as FT_Long, - face_index, &mut face); - - if !succeeded(result) || face.is_null() { - return Err(()); - } - if let Some(s) = pt_size { - FontHandle::set_char_size(face, s).or(Err(()))? - } - Ok(face) - } - } + let mut handle = FontHandle { + face: face, + font_data: template.clone(), + handle: fctx.clone(), + can_do_fast_shaping: false, + }; + // TODO (#11310): Implement basic support for GPOS and GSUB. + handle.can_do_fast_shaping = handle.has_table(KERN) && + !handle.has_table(GPOS) && + !handle.has_table(GSUB); + Ok(handle) } fn template(&self) -> Arc { diff --git a/components/gfx/platform/freetype/font_template.rs b/components/gfx/platform/freetype/font_template.rs index 13209e3a016..c4b4a9bc9e7 100644 --- a/components/gfx/platform/freetype/font_template.rs +++ b/components/gfx/platform/freetype/font_template.rs @@ -16,34 +16,21 @@ use webrender_api::NativeFontHandle; pub struct FontTemplateData { // If you add members here, review the Debug impl below - pub bytes: Vec, + pub bytes: Option>, pub identifier: Atom, } impl fmt::Debug for FontTemplateData { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { fmt.debug_struct("FontTemplateData") - .field("bytes", &format!("[{} bytes]", self.bytes.len())) + .field("bytes", &self.bytes.as_ref().map(|b| format!("[{} bytes]", b.len()))) .field("identifier", &self.identifier) .finish() } } impl FontTemplateData { - pub fn new(identifier: Atom, font_data: Option>) -> Result { - let bytes = match font_data { - Some(bytes) => { - bytes - }, - None => { - // TODO: Handle file load failure! - let mut file = File::open(&*identifier)?; - let mut buffer = vec![]; - file.read_to_end(&mut buffer).unwrap(); - buffer - }, - }; - + pub fn new(identifier: Atom, bytes: Option>) -> Result { Ok(FontTemplateData { bytes: bytes, identifier: identifier, @@ -54,17 +41,29 @@ impl FontTemplateData { /// operation (depending on the platform) which performs synchronous disk I/O /// and should never be done lightly. pub fn bytes(&self) -> Vec { - self.bytes.clone() + self.bytes_if_in_memory().unwrap_or_else(|| { + let mut file = File::open(&*self.identifier).expect("Couldn't open font file!"); + let mut buffer = vec![]; + file.read_to_end(&mut buffer).unwrap(); + buffer + }) } /// Returns a clone of the bytes in this font if they are in memory. This function never /// performs disk I/O. pub fn bytes_if_in_memory(&self) -> Option> { - Some(self.bytes()) + self.bytes.clone() } /// Returns the native font that underlies this font template, if applicable. pub fn native_font(&self) -> Option { - None + if self.bytes.is_none() { + Some(NativeFontHandle { + pathname: String::from(&*self.identifier), + index: 0, + }) + } else { + None + } } }