[fonts] Stop loading all fonts into memory at service startup
- AssetCollection now holds a bidirectional map of buffer IDs and paths to font files
- Removed AssetCollection.assets cache in favor of relying on filesystem behavior (VFS can already manage caching and handle-counting)
- Expect slightly more disk reads (previously: read all files exactly once; now: read all files once at startup + individually as needed)
I18N-15 #done
Change-Id: Ia0b1bfe2ed9336ffc140aaaf8d19d1bb6998b001
diff --git a/garnet/bin/fonts/src/font_service.rs b/garnet/bin/fonts/src/font_service.rs
index 5c20e59..2e7d06e 100644
--- a/garnet/bin/fonts/src/font_service.rs
+++ b/garnet/bin/fonts/src/font_service.rs
@@ -12,51 +12,33 @@
use fidl_fuchsia_fonts as fonts;
use fidl_fuchsia_mem as mem;
use fuchsia_async as fasync;
-use fuchsia_zircon as zx;
-use fuchsia_zircon::HandleBased;
use futures::prelude::*;
use futures::{future, Future, FutureExt};
use log::error;
use std::collections::BTreeMap;
use std::fs::File;
use std::path::{Path, PathBuf};
-use std::sync::{Arc, RwLock};
+use std::sync::Arc;
use unicase::UniCase;
-fn clone_buffer(buf: &mem::Buffer) -> Result<mem::Buffer, Error> {
- let vmo_rights = zx::Rights::BASIC | zx::Rights::READ | zx::Rights::MAP;
- let vmo = buf.vmo.duplicate_handle(vmo_rights).context("Failed to duplicate VMO handle.")?;
- Ok(mem::Buffer { vmo, size: buf.size })
-}
-
-struct Asset {
- path: PathBuf,
- buffer: RwLock<Option<mem::Buffer>>,
-}
-
-/// Provides cached storage of [`Asset`]s. Should be initialized by [`FontService`].
+/// Stores the relationship between [`Asset`] paths and IDs.
+/// Should be initialized by [`FontService`].
///
-/// Each [`Asset`] is assigned an ID equal to its index in `assets`.
-///
-/// # Examples
+/// `path_to_id_map` and `id_to_path_map` form a bidirectional map, so this relation holds:
/// ```
-/// let asset_collection = AssetCollection::new();
-/// // Load data into asset_collection, including "/path/to/font.ttf".
-/// let path: PathBuf = PathBuf::from("/path/to/font.ttf");
-/// let asset_id: u32 = asset_collection.assets_map.get(path).unwrap();
-/// let asset: &Asset = &asset_collection.assets[asset_id as usize];
+/// assert_eq!(self.path_to_id_map.get(&path), Some(&id));
+/// assert_eq!(self.id_to_path_map.get(&id), Some(&path));
/// ```
struct AssetCollection {
/// Maps [`Asset`] path to ID.
- assets_map: BTreeMap<PathBuf, u32>,
-
- /// [`Asset`] cache. Indices correspond to values stored in `self.assets_map`.
- ///
- /// Currently, no cache eviction strategy is implemented (I18N-15).
- assets: Vec<Asset>,
+ path_to_id_map: BTreeMap<PathBuf, u32>,
+ /// Inverse of `path_to_id_map`.
+ id_to_path_map: BTreeMap<u32, PathBuf>,
+ /// Next ID to assign, autoincremented from 0.
+ next_id: u32,
}
-/// Load a `Vmo` from disk into a `Buffer`.
+/// Get `VMO` handle to the [`Asset`] at `path`.
fn load_asset_to_vmo(path: &Path) -> Result<mem::Buffer, Error> {
let file = File::open(path)?;
let vmo = fdio::get_vmo_copy_from_file(&file)?;
@@ -66,48 +48,38 @@
impl AssetCollection {
fn new() -> AssetCollection {
- AssetCollection { assets_map: BTreeMap::new(), assets: vec![] }
+ AssetCollection {
+ path_to_id_map: BTreeMap::new(),
+ id_to_path_map: BTreeMap::new(),
+ next_id: 0,
+ }
}
- /// Lazily add the [`Asset`] found at `path` to the collection and return its ID.
- /// If it already exists, just return the ID.
+ /// Add the [`Asset`] found at `path` to the collection and return its ID.
+ /// If `path` is already in the collection, return the existing ID.
///
- /// The push onto `assets` is lazy in the sense that the pushed [`Asset`] has an empty `buffer`.
+ /// TODO(seancuff): Switch to updating ID of existing entries. This would allow assets to be
+ /// updated without restarting the service (e.g. installing a newer version of a file). Clients
+ /// would need to check the ID of their currently-held asset against the response.
fn add_or_get_asset_id(&mut self, path: &Path) -> u32 {
- if let Some(id) = self.assets_map.get(path) {
+ if let Some(id) = self.path_to_id_map.get(&path.to_path_buf()) {
return *id;
}
-
- let id = self.assets.len() as u32;
- self.assets.push(Asset { path: path.to_path_buf(), buffer: RwLock::new(None) });
- self.assets_map.insert(path.to_path_buf(), id);
+ let id = self.next_id;
+ self.id_to_path_map.insert(id, path.to_path_buf());
+ self.path_to_id_map.insert(path.to_path_buf(), id);
+ self.next_id += 1;
id
}
- /// Get a `Buffer` holding the `Vmo` for the [`Asset`] corresponding to `id`, using the
- /// cache if possible. On a cache miss, the [`Asset`] is cached unconditionally.
+ /// Get a `Buffer` holding the `Vmo` for the [`Asset`] corresponding to `id`.
fn get_asset(&self, id: u32) -> Result<mem::Buffer, Error> {
- assert!(id < self.assets.len() as u32);
-
- let asset = &self.assets[id as usize];
-
- if let Some(cached) = asset.buffer.read().unwrap().as_ref() {
- return clone_buffer(cached);
+ if let Some(path) = self.id_to_path_map.get(&id) {
+ let buf = load_asset_to_vmo(path)
+ .with_context(|_| format!("Failed to load {}", path.to_string_lossy()))?;
+ return Ok(buf);
}
-
- let buf = load_asset_to_vmo(&asset.path)
- .with_context(|_| format!("Failed to load {}", asset.path.to_string_lossy()))?;
- let buf_clone = clone_buffer(&buf)?;
- *asset.buffer.write().unwrap() = Some(buf);
- Ok(buf_clone)
- }
-
- /// Clear the cache by deallocating all [`Asset`]'s `buffer`s.
- /// Does not reduce the size of `assets`.
- fn reset_cache(&mut self) {
- for asset in self.assets.iter_mut() {
- *asset.buffer.write().unwrap() = None;
- }
+ Err(format_err!("No asset found with id {}", id))
}
}
@@ -259,10 +231,6 @@
}
}
}
-
- // Flush the cache. Font files will be loaded again when they are needed.
- self.assets.reset_cache();
-
Ok(())
}
diff --git a/garnet/bin/fonts/tests/font_provider_test.rs b/garnet/bin/fonts/tests/font_provider_test.rs
index b2a8226..621545b 100644
--- a/garnet/bin/fonts/tests/font_provider_test.rs
+++ b/garnet/bin/fonts/tests/font_provider_test.rs
@@ -14,6 +14,19 @@
use fuchsia_zircon as zx;
use fuchsia_zircon::AsHandleRef;
+macro_rules! assert_buf_eq {
+ ($font_info_a:ident, $font_info_b:ident) => {
+ assert!(
+ $font_info_a.buffer_id == $font_info_b.buffer_id,
+ "{}.buffer_id == {}.buffer_id\n{0}: {:?}\n{1}: {:?}",
+ stringify!($font_info_a),
+ stringify!($font_info_b),
+ $font_info_a,
+ $font_info_b
+ )
+ };
+}
+
#[derive(Debug, Eq, PartialEq)]
struct FontInfo {
vmo_koid: zx::Koid,
@@ -23,7 +36,9 @@
}
async fn get_font_info(
- font_provider: &fonts::ProviderProxy, name: Option<String>, language: Option<Vec<String>>,
+ font_provider: &fonts::ProviderProxy,
+ name: Option<String>,
+ language: Option<Vec<String>>,
character: char,
) -> Result<FontInfo, Error> {
let font = await!(font_provider.get_font(&mut fonts::Request {
@@ -51,7 +66,8 @@
}
async fn get_font_info_basic(
- font_provider: &fonts::ProviderProxy, name: Option<String>,
+ font_provider: &fonts::ProviderProxy,
+ name: Option<String>,
) -> Result<FontInfo, Error> {
await!(get_font_info(font_provider, name, None, '\0'))
}
@@ -75,22 +91,16 @@
let default =
await!(get_font_info_basic(&font_provider, None)).context("Failed to load default font")?;
- let roboto = await!(get_font_info_basic(
- &font_provider,
- Some("Roboto".to_string())
- ))
- .context("Failed to load Roboto")?;
- let material_icons = await!(get_font_info_basic(
- &font_provider,
- Some("Material Icons".to_string())
- ))
- .context("Failed to load Material Icons")?;
+ let roboto = await!(get_font_info_basic(&font_provider, Some("Roboto".to_string())))
+ .context("Failed to load Roboto")?;
+ let material_icons =
+ await!(get_font_info_basic(&font_provider, Some("Material Icons".to_string())))
+ .context("Failed to load Material Icons")?;
// Roboto should be returned by default.
- assert!(default == roboto);
+ assert_buf_eq!(default, roboto);
// Material Icons request should return a different font.
- assert!(default.vmo_koid != material_icons.vmo_koid);
assert!(default.buffer_id != material_icons.buffer_id);
Ok(())
@@ -102,17 +112,14 @@
let (_app, font_provider) = start_provider_with_default_fonts()?;
// Both requests should return the same font.
- let materialicons = await!(get_font_info_basic(
- &font_provider,
- Some("MaterialIcons".to_string())
- ))
- .context("Failed to load MaterialIcons")?;
- let material_icons = await!(get_font_info_basic(
- &font_provider,
- Some("Material Icons".to_string())
- ))
- .context("Failed to load Material Icons")?;
- assert!(materialicons == material_icons);
+ let materialicons =
+ await!(get_font_info_basic(&font_provider, Some("MaterialIcons".to_string())))
+ .context("Failed to load MaterialIcons")?;
+ let material_icons =
+ await!(get_font_info_basic(&font_provider, Some("Material Icons".to_string())))
+ .context("Failed to load Material Icons")?;
+
+ assert_buf_eq!(materialicons, material_icons);
Ok(())
}
@@ -163,8 +170,16 @@
))
.context("Failed to load NotoSansCJK font")?;
- assert!(noto_sans_cjk_ja.vmo_koid == noto_sans_cjk_sc.vmo_koid);
- assert!(noto_sans_cjk_ja.index != noto_sans_cjk_sc.index);
+ assert_buf_eq!(noto_sans_cjk_ja, noto_sans_cjk_sc);
+
+ assert!(
+ noto_sans_cjk_ja.index != noto_sans_cjk_sc.index,
+ "noto_sans_cjk_ja.index != noto_sans_cjk_sc.index\n \
+ noto_sans_cjk_ja.index: {:?}\n \
+ noto_sans_cjk_sc.index: {:?}",
+ noto_sans_cjk_ja,
+ noto_sans_cjk_sc
+ );
Ok(())
}
@@ -191,7 +206,7 @@
.context("Failed to load NotoSansCJK font")?;
// Same font should be returned in both cases.
- assert!(noto_sans_cjk_ja == noto_sans_cjk_ja_by_char);
+ assert_buf_eq!(noto_sans_cjk_ja, noto_sans_cjk_ja_by_char);
Ok(())
}
@@ -221,7 +236,7 @@
// The query above requested Roboto Slab, so it's expected to return
// Noto Serif CJK instead of Noto Sans CJK because Roboto Slab and
// Noto Serif CJK are both in serif fallback group.
- assert!(noto_serif_cjk_ja == noto_serif_cjk_ja_by_char);
+ assert_buf_eq!(noto_serif_cjk_ja, noto_serif_cjk_ja_by_char);
Ok(())
}
