Remove FontContextHandle

The `FontContextHandle` was really only used on FreeType platforms to store the `FT_Library` handle to use for creating faces. Each `FontContext` and `FontCacheThread` would create its own `FontContextHandle`. This change removes this data structure in favor of a mutex-protected shared `FontContextHandle` for an entire Servo process. The handle is initialized using a `OnceLock` to ensure that it only happens once and also that it stays alive for the entire process lifetime. In addition to greatly simplifying the code, this will make it possible for different threads to share platform-specific `FontHandle`s, avoiding multiple allocations for a single font. The only downside to all of this is that memory usage of FreeType fonts isn't measured (though the mechanism is still there). This is because the `FontCacheThread` currently doesn't do any memory measurement. Eventually this *will* happen though, during the font system redesign. In exchange, this should reduce the memory usage since there is only a single FreeType library loaded into memory now. This is part of servo#32033.
mrobinson · Apr 12, 2024 · 443409f · 443409f
1 parent 89a4820
commit 443409f
Show file tree

Hide file tree

Showing 16 changed files with 152 additions and 247 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/components/gfx/Cargo.toml b/components/gfx/Cargo.toml
@@ -27,6 +27,7 @@ libc = { workspace = true }
 log = { workspace = true }
 malloc_size_of = { workspace = true }
 net_traits = { workspace = true }
+parking_lot = { workspace = true }
 range = { path = "../range" }
 serde = { workspace = true }
 servo_arc = { workspace = true }

diff --git a/components/gfx/font.rs b/components/gfx/font.rs
@@ -28,7 +28,6 @@ use crate::font_cache_thread::FontIdentifier;
 use crate::font_context::{FontContext, FontSource};
 use crate::font_template::FontTemplateDescriptor;
 use crate::platform::font::{FontHandle, FontTable};
-use crate::platform::font_context::FontContextHandle;
 pub use crate::platform::font_list::fallback_font_families;
 use crate::platform::font_template::FontTemplateData;
 use crate::text::glyph::{ByteIndex, GlyphData, GlyphId, GlyphStore};
@@ -56,7 +55,6 @@ static TEXT_SHAPING_PERFORMANCE_COUNTER: AtomicUsize = AtomicUsize::new(0);
 
 pub trait FontHandleMethods: Sized {
     fn new_from_template(
-        fctx: &FontContextHandle,
         template: Arc<FontTemplateData>,
         pt_size: Option<Au>,
     ) -> Result<Self, &'static str>;

diff --git a/components/gfx/font_cache_thread.rs b/components/gfx/font_cache_thread.rs
@@ -26,7 +26,6 @@ use webrender_api::{FontInstanceKey, FontKey};
 use crate::font::{FontFamilyDescriptor, FontFamilyName, FontSearchScope};
 use crate::font_context::FontSource;
 use crate::font_template::{FontTemplate, FontTemplateDescriptor};
-use crate::platform::font_context::FontContextHandle;
 use crate::platform::font_list::{
     for_each_available_family, for_each_variation, system_default_family, LocalFontIdentifier,
     SANS_SERIF_FONT_FAMILY,
@@ -75,13 +74,12 @@ impl FontTemplates {
     pub fn find_font_for_style(
         &mut self,
         desc: &FontTemplateDescriptor,
-        fctx: &FontContextHandle,
     ) -> Option<Arc<FontTemplateData>> {
         // TODO(Issue #189): optimize lookup for
         // regular/bold/italic/bolditalic with fixed offsets and a
         // static decision table for fallback between these values.
         for template in &mut self.templates {
-            let maybe_template = template.data_for_descriptor(fctx, desc);
+            let maybe_template = template.data_for_descriptor(desc);
             if maybe_template.is_some() {
                 return maybe_template;
             }
@@ -91,8 +89,7 @@ impl FontTemplates {
         // TODO(#190): Do a better job.
         let (mut best_template_data, mut best_distance) = (None, f32::MAX);
         for template in &mut self.templates {
-            if let Some((template_data, distance)) =
-                template.data_for_approximate_descriptor(fctx, desc)
+            if let Some((template_data, distance)) = template.data_for_approximate_descriptor(desc)
             {
                 if distance < best_distance {
                     best_template_data = Some(template_data);
@@ -159,7 +156,6 @@ struct FontCache {
     generic_fonts: HashMap<FontFamilyName, LowercaseString>,
     local_families: HashMap<LowercaseString, FontTemplates>,
     web_families: HashMap<LowercaseString, FontTemplates>,
-    font_context: FontContextHandle,
     core_resource_thread: CoreResourceThread,
     webrender_api: Box<dyn WebrenderApi>,
     webrender_fonts: HashMap<FontIdentifier, FontKey>,
@@ -404,7 +400,7 @@ impl FontCache {
             // TODO(Issue #192: handle generic font families, like 'serif' and 'sans-serif'.
             // if such family exists, try to match style to a font
 
-            s.find_font_for_style(template_descriptor, &self.font_context)
+            s.find_font_for_style(template_descriptor)
         } else {
             debug!(
                 "FontList: Couldn't find font family with name={}",
@@ -423,7 +419,7 @@ impl FontCache {
 
         if self.web_families.contains_key(&family_name) {
             let templates = self.web_families.get_mut(&family_name).unwrap();
-            templates.find_font_for_style(template_descriptor, &self.font_context)
+            templates.find_font_for_style(template_descriptor)
         } else {
             None
         }
@@ -498,7 +494,6 @@ impl FontCacheThread {
                     generic_fonts,
                     local_families: HashMap::new(),
                     web_families: HashMap::new(),
-                    font_context: FontContextHandle::default(),
                     core_resource_thread,
                     webrender_api,
                     webrender_fonts: HashMap::new(),

diff --git a/components/gfx/font_context.rs b/components/gfx/font_context.rs
@@ -12,7 +12,6 @@ use std::sync::atomic::{AtomicUsize, Ordering};
 use app_units::Au;
 use fnv::FnvHasher;
 use log::debug;
-use malloc_size_of::{MallocSizeOf, MallocSizeOfOps};
 use servo_arc::Arc;
 use style::computed_values::font_variant_caps::T as FontVariantCaps;
 use style::properties::style_structs::Font as FontStyleStruct;
@@ -24,7 +23,6 @@ use crate::font::{
 use crate::font_cache_thread::FontTemplateInfo;
 use crate::font_template::FontTemplateDescriptor;
 use crate::platform::font::FontHandle;
-pub use crate::platform::font_context::FontContextHandle;
 
 static SMALL_CAPS_SCALE_FACTOR: f32 = 0.8; // Matches FireFox (see gfxFont.h)
 
@@ -48,7 +46,6 @@ pub trait FontSource {
 /// required.
 #[derive(Debug)]
 pub struct FontContext<S: FontSource> {
-    platform_handle: FontContextHandle,
     font_source: S,
 
     // TODO: The font context holds a strong ref to the cached fonts
@@ -66,9 +63,7 @@ pub struct FontContext<S: FontSource> {
 impl<S: FontSource> FontContext<S> {
     pub fn new(font_source: S) -> FontContext<S> {
         #[allow(clippy::default_constructed_unit_structs)]
-        let handle = FontContextHandle::default();
         FontContext {
-            platform_handle: handle,
             font_source,
             font_cache: HashMap::new(),
             font_template_cache: HashMap::new(),
@@ -217,11 +212,7 @@ impl<S: FontSource> FontContext<S> {
         descriptor: FontDescriptor,
         synthesized_small_caps: Option<FontRef>,
     ) -> Result<Font, &'static str> {
-        let handle = FontHandle::new_from_template(
-            &self.platform_handle,
-            info.font_template,
-            Some(descriptor.pt_size),
-        )?;
+        let handle = FontHandle::new_from_template(info.font_template, Some(descriptor.pt_size))?;
 
         let font_instance_key = self
             .font_source
@@ -235,13 +226,6 @@ impl<S: FontSource> FontContext<S> {
     }
 }
 
-impl<S: FontSource> MallocSizeOf for FontContext<S> {
-    fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
-        // FIXME(njn): Measure other fields eventually.
-        self.platform_handle.size_of(ops)
-    }
-}
-
 #[derive(Debug, Eq, Hash, PartialEq)]
 struct FontCacheKey {
     font_descriptor: FontDescriptor,

diff --git a/components/gfx/font_template.rs b/components/gfx/font_template.rs
@@ -15,7 +15,6 @@ use style::values::computed::font::FontWeight;
 use crate::font::FontHandleMethods;
 use crate::font_cache_thread::FontIdentifier;
 use crate::platform::font::FontHandle;
-use crate::platform::font_context::FontContextHandle;
 use crate::platform::font_template::FontTemplateData;
 
 /// Describes how to select a font from a given family. This is very basic at the moment and needs
@@ -130,18 +129,15 @@ impl FontTemplate {
     }
 
     /// Get the descriptor. Returns `None` when instantiating the data fails.
-    pub fn descriptor(
-        &mut self,
-        font_context: &FontContextHandle,
-    ) -> Option<FontTemplateDescriptor> {
+    pub fn descriptor(&mut self) -> Option<FontTemplateDescriptor> {
         // The font template data can be unloaded when nothing is referencing
         // it (via the Weak reference to the Arc above). However, if we have
         // already loaded a font, store the style information about it separately,
         // so that we can do font matching against it again in the future
         // without having to reload the font (unless it is an actual match).
 
         self.descriptor.or_else(|| {
-            if self.instantiate(font_context).is_err() {
+            if self.instantiate().is_err() {
                 return None;
             };
 
@@ -155,10 +151,9 @@ impl FontTemplate {
     /// Get the data for creating a font if it matches a given descriptor.
     pub fn data_for_descriptor(
         &mut self,
-        fctx: &FontContextHandle,
         requested_desc: &FontTemplateDescriptor,
     ) -> Option<Arc<FontTemplateData>> {
-        self.descriptor(fctx).and_then(|descriptor| {
+        self.descriptor().and_then(|descriptor| {
             if *requested_desc == descriptor {
                 self.data().ok()
             } else {
@@ -171,23 +166,22 @@ impl FontTemplate {
     /// descriptor, if the font can be loaded.
     pub fn data_for_approximate_descriptor(
         &mut self,
-        font_context: &FontContextHandle,
         requested_descriptor: &FontTemplateDescriptor,
     ) -> Option<(Arc<FontTemplateData>, f32)> {
-        self.descriptor(font_context).and_then(|descriptor| {
+        self.descriptor().and_then(|descriptor| {
             self.data()
                 .ok()
                 .map(|data| (data, descriptor.distance_from(requested_descriptor)))
         })
     }
 
-    fn instantiate(&mut self, font_context: &FontContextHandle) -> Result<(), &'static str> {
+    fn instantiate(&mut self) -> Result<(), &'static str> {
         if !self.is_valid {
             return Err("Invalid font template");
         }
 
         let data = self.data().map_err(|_| "Could not get FontTemplate data")?;
-        let handle = FontHandleMethods::new_from_template(font_context, data, None);
+        let handle = FontHandleMethods::new_from_template(data, None);
         self.is_valid = handle.is_ok();
         let handle: FontHandle = handle?;
         self.descriptor = Some(FontTemplateDescriptor::new(

diff --git a/components/gfx/platform/freetype/font.rs b/components/gfx/platform/freetype/font.rs
@@ -10,7 +10,7 @@ use std::{mem, ptr};
 use app_units::Au;
 use freetype::freetype::{
     FT_Done_Face, FT_F26Dot6, FT_Face, FT_FaceRec, FT_Get_Char_Index, FT_Get_Kerning,
-    FT_Get_Postscript_Name, FT_Get_Sfnt_Table, FT_GlyphSlot, FT_Int32, FT_Kerning_Mode, FT_Library,
+    FT_Get_Postscript_Name, FT_Get_Sfnt_Table, FT_GlyphSlot, FT_Int32, FT_Kerning_Mode,
     FT_Load_Glyph, FT_Load_Sfnt_Table, FT_Long, FT_New_Face, FT_New_Memory_Face, FT_Set_Char_Size,
     FT_Sfnt_Tag, FT_SizeRec, FT_Size_Metrics, FT_UInt, FT_ULong, FT_Vector, FT_STYLE_FLAG_ITALIC,
 };
@@ -22,12 +22,12 @@ use style::computed_values::font_weight::T as FontWeight;
 use style::values::computed::font::FontStyle;
 
 use super::c_str_to_string;
+use super::library_handle::FreeTypeLibraryHandle;
 use crate::font::{
     FontHandleMethods, FontMetrics, FontTableMethods, FontTableTag, FractionalPixel, GPOS, GSUB,
     KERN,
 };
 use crate::font_cache_thread::FontIdentifier;
-use crate::platform::font_context::FontContextHandle;
 use crate::platform::font_template::FontTemplateData;
 use crate::text::glyph::GlyphId;
 use crate::text::util::fixed_to_float;
@@ -76,37 +76,35 @@ pub struct FontHandle {
     // if the font is created using FT_Memory_Face.
     font_data: Arc<FontTemplateData>,
     face: FT_Face,
-    // `context_handle` is unused, but here to ensure that the underlying
-    // FreeTypeLibraryHandle is not dropped.
-    context_handle: FontContextHandle,
     can_do_fast_shaping: bool,
 }
 
 impl Drop for FontHandle {
     fn drop(&mut self) {
         assert!(!self.face.is_null());
         unsafe {
+            // The FreeType documentation says that both `FT_New_Face` and `FT_Done_Face`
+            // should be protected by a mutex.
+            // See https://freetype.org/freetype2/docs/reference/ft2-library_setup.html.
+            let _guard = FreeTypeLibraryHandle::get().lock();
             if !succeeded(FT_Done_Face(self.face)) {
                 panic!("FT_Done_Face failed");
             }
         }
     }
 }
 
-fn create_face(
-    lib: FT_Library,
-    template: &FontTemplateData,
-    pt_size: Option<Au>,
-) -> Result<FT_Face, &'static str> {
+fn create_face(template: &FontTemplateData, pt_size: Option<Au>) -> Result<FT_Face, &'static str> {
     unsafe {
         let mut face: FT_Face = ptr::null_mut();
         let face_index = 0 as FT_Long;
+        let library = FreeTypeLibraryHandle::get().lock();
 
         let result = match template.identifier {
             FontIdentifier::Web(_) => {
                 let bytes = template.bytes();
                 FT_New_Memory_Face(
-                    lib,
+                    library.freetype_library,
                     bytes.as_ptr(),
                     bytes.len() as FT_Long,
                     face_index,
@@ -120,7 +118,12 @@ fn create_face(
                 // https://github.com/servo/servo/pull/20506#issuecomment-378838800
                 let filename =
                     CString::new(&*local_identifier.path).expect("filename contains NUL byte!");
-                FT_New_Face(lib, filename.as_ptr(), face_index, &mut face)
+                FT_New_Face(
+                    library.freetype_library,
+                    filename.as_ptr(),
+                    face_index,
+                    &mut face,
+                )
             },
         };
 
@@ -138,21 +141,13 @@ fn create_face(
 
 impl FontHandleMethods for FontHandle {
     fn new_from_template(
-        fctx: &FontContextHandle,
         template: Arc<FontTemplateData>,
         pt_size: Option<Au>,
     ) -> Result<FontHandle, &'static str> {
-        let ft_ctx: FT_Library = fctx.ctx.ctx;
-        if ft_ctx.is_null() {
-            return Err("Null FT_Library");
-        }
-
-        let face = create_face(ft_ctx, &template, pt_size)?;
-
+        let face = create_face(&template, pt_size)?;
         let mut handle = FontHandle {
             face,
             font_data: template,
-            context_handle: fctx.clone(),
             can_do_fast_shaping: false,
         };
         // TODO (#11310): Implement basic support for GPOS and GSUB.