From 6e6634e175fc2772f007ba2a6654180850c74253 Mon Sep 17 00:00:00 2001 From: Luke Seelenbinder Date: Wed, 27 Oct 2021 10:02:07 +0200 Subject: [PATCH] Performance, Memory and Cleanup Improvements (#5) * Improve performance, ergonomics, and memory usage. * Improves `get_font_stack` by using `spawn_blocking`. * Adjusts `get_font_stack` to take `font_names` as a `&[&str]` instead of `Vec`. * Improves `load_glyphs` by using `spawn_blocking` instead of async file operations. * Improves memory usage of `combine_glyphs` by eliminating most copies. * Fixes a bug in `combine_glyphs` where the end and start of the range was the same. * Move to Edition 2021 and bump dependencies and version. * Bump `sdf_glyph_renderer` dependency. --- Cargo.toml | 15 +++---- src/lib.rs | 91 +++++++++++++++++++------------------- tests/integration_tests.rs | 16 +++---- 3 files changed, 57 insertions(+), 65 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 05d5f1d..c3309a0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,14 +1,14 @@ [package] name = "pbf_font_tools" -version = "1.0.2" -authors = ["Ian Wagner "] +version = "2.0.0" +authors = ["Ian Wagner ", "Luke Seelenbinder "] license = "BSD-3-Clause" repository = "https://github.com/stadiamaps/pbf_font_tools" readme = "README.md" description = "Tools for working with SDF font glyphs encoded in protobuf format." keywords = ["sdf", "protobuf", "fonts"] categories = ["encoding", "parsing", "rendering::data-formats"] -edition = "2018" +edition = "2021" [features] freetype = ["freetype-rs", "sdf_glyph_renderer/freetype"] @@ -18,21 +18,20 @@ futures = "0.3.14" protobuf = "2.22.1" [dependencies.tokio] -version = "1.5.0" -features = ["fs", "io-util"] +version = "1.12.0" [build-dependencies] glob = "0.3.0" protobuf-codegen-pure = "2.22.1" [dev-dependencies.tokio] -version = "1.5.0" +version = "1.12.0" features = ["fs", "io-util", "macros", "rt"] [dependencies.freetype-rs] -version = "0.27.0" +version = "0.28.0" optional = true [dependencies.sdf_glyph_renderer] -version = "0.1.3" +version = "0.2.0" optional = true diff --git a/src/lib.rs b/src/lib.rs index bc08d56..d9a7146 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -10,22 +10,18 @@ //! * https://github.com/mapbox/glyph-pbf-composite //! * https://github.com/klokantech/tileserver-gl/blob/master/src/utils.js -use std::collections::HashSet; -use std::path::Path; +pub use proto::glyphs; use futures::future::try_join_all; - -use tokio::fs::File; -use tokio::io::AsyncReadExt; +use protobuf::Message; +use std::{collections::HashSet, fs::File, path::Path}; +use tokio::task::spawn_blocking; #[cfg(feature = "freetype")] pub mod generate; mod proto; -pub use proto::glyphs; -use protobuf::Message; - type GlyphResult = Result; /// Generates a single combined font stack for the set of fonts provided. @@ -35,35 +31,34 @@ type GlyphResult = Result; /// even if the loaded range is empty for a given font. pub async fn get_font_stack( font_path: &Path, - font_names: Vec, + font_names: &[&str], start: u32, end: u32, ) -> GlyphResult { - // Load fonts (futures) - let mut load_futures: Vec<_> = vec![]; - for font in font_names.iter() { - load_futures.push(load_glyphs(font_path, font, start, end)); - } + // Load fonts + let glyph_data = try_join_all( + font_names + .iter() + .map(|font| load_glyphs(font_path, font, start, end)), + ) + .await?; // Combine all the glyphs into a single instance, using the ordering to determine priority. - match try_join_all(load_futures).await { - Ok(data) => { - if let Some(result) = combine_glyphs(data) { - Ok(result) - } else { - // Construct an empty message manually if the range is not covered - let mut result = glyphs::glyphs::new(); - - let mut stack = glyphs::fontstack::new(); - stack.set_name(font_names.join(", ")); - stack.set_range(format!("{}-{}", start, end)); - - result.mut_stacks().push(stack); - Ok(result) - } - } - Err(e) => Err(e), - } + // This can take some time, so mark it blocking. + Ok(spawn_blocking(move || combine_glyphs(glyph_data)) + .await + .unwrap() // Unwrap any panics. + .unwrap_or_else(|| { + // Construct an empty message manually if the range is not covered + let mut result = glyphs::glyphs::new(); + + let mut stack = glyphs::fontstack::new(); + stack.set_name(font_names.join(", ")); + stack.set_range(format!("{}-{}", start, end)); + + result.mut_stacks().push(stack); + result + })) } /// Loads a single font PBF slice from disk. @@ -73,11 +68,15 @@ pub async fn load_glyphs(font_path: &Path, font_name: &str, start: u32, end: u32 let full_path = font_path .join(font_name) .join(format!("{}-{}.pbf", start, end)); - let mut file = File::open(full_path).await?; - let mut buf = Vec::new(); - file.read_to_end(&mut buf).await?; - Message::parse_from_bytes(&buf) + // Note: Counter-intuitively, it's much faster to use blocking IO with `spawn_blocking` here, + // since the `Message::parse_` call will block as well. + spawn_blocking(|| { + let mut file = File::open(full_path)?; + Message::parse_from_reader(&mut file) + }) + .await + .unwrap() // Unwrap any panics. } /// Combines a list of SDF font glyphs into a single glyphs message. @@ -87,32 +86,32 @@ pub async fn load_glyphs(font_path: &Path, font_name: &str, start: u32, end: u32 /// /// NOTE: This returns `None` if there are no glyphs in the range. If you need to /// construct an empty message, the responsibility lies with the caller. -pub fn combine_glyphs(data: Vec) -> Option { +pub fn combine_glyphs(glyphs_to_combine: Vec) -> Option { let mut result = glyphs::glyphs::new(); let mut combined_stack = glyphs::fontstack::new(); let mut coverage: HashSet = HashSet::new(); - for glyph_stack in data { - for font_stack in glyph_stack.get_stacks() { + for mut glyph_stack in glyphs_to_combine { + for mut font_stack in glyph_stack.take_stacks() { if !combined_stack.has_name() { - combined_stack.set_name(String::from(font_stack.get_name())); + combined_stack.set_name(font_stack.take_name()); } else { - let stack_name = combined_stack.get_name().to_owned(); - let this_name = font_stack.get_name(); - combined_stack.set_name(format!("{}, {}", stack_name, this_name)); + let name = combined_stack.mut_name(); + name.push_str(", "); + name.push_str(font_stack.get_name()); } - for glyph in font_stack.get_glyphs() { + for glyph in font_stack.take_glyphs() { if !coverage.contains(&glyph.get_id()) { coverage.insert(glyph.get_id()); - combined_stack.mut_glyphs().push(glyph.to_owned()); + combined_stack.mut_glyphs().push(glyph); } } } } let start = coverage.iter().min()?; - let end = coverage.iter().min()?; + let end = coverage.iter().max()?; combined_stack.set_range(format!("{}-{}", start, end)); diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 3500c19..d0d2fc2 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -24,22 +24,16 @@ async fn test_load_glyphs() { #[tokio::test] async fn test_get_font_stack() { let font_path = Path::new("tests").join("glyphs"); - let font_names = vec![ - String::from("SeoulNamsan L"), - String::from("Open Sans Light"), - ]; + let font_names = vec!["SeoulNamsan L", "Open Sans Light"]; - let namsan_font_future = - pbf_font_tools::load_glyphs(font_path.as_path(), "SeoulNamsan L", 0, 255); + let namsan_font = pbf_font_tools::load_glyphs(font_path.as_path(), "SeoulNamsan L", 0, 255); - let open_sans_font_future = + let open_sans_font = pbf_font_tools::load_glyphs(font_path.as_path(), "Open Sans Light", 0, 255); - let font_stack_fut = pbf_font_tools::get_font_stack(font_path.as_path(), font_names, 0, 255); + let font_stack = pbf_font_tools::get_font_stack(font_path.as_path(), &font_names, 0, 255); - let result = join3(namsan_font_future, open_sans_font_future, font_stack_fut).await; - - match result { + match join3(namsan_font, open_sans_font, font_stack).await { (Ok(namsan_glyphs), Ok(open_sans_glyphs), Ok(combined_glyphs)) => { // Make sure we have a font stack, and that it has the expected name // and glyph count.