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<String>`.
* 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

@@ -1,14 +1,14 @@
 [package]
 name = "pbf_font_tools"
-version = "1.0.2"
-authors = ["Ian Wagner <[email protected]>"]
+version = "2.0.0"
+authors = ["Ian Wagner <[email protected]>", "Luke Seelenbinder <[email protected]>"]
 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

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<glyphs::glyphs, protobuf::ProtobufError>;
 
 /// Generates a single combined font stack for the set of fonts provided.
@@ -35,35 +31,34 @@ type GlyphResult = Result<glyphs::glyphs, protobuf::ProtobufError>;
 /// even if the loaded range is empty for a given font.
 pub async fn get_font_stack(
     font_path: &Path,
-    font_names: Vec<String>,
+    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<glyphs::glyphs>) -> Option<glyphs::glyphs> {
+pub fn combine_glyphs(glyphs_to_combine: Vec<glyphs::glyphs>) -> Option<glyphs::glyphs> {
     let mut result = glyphs::glyphs::new();
     let mut combined_stack = glyphs::fontstack::new();
     let mut coverage: HashSet<u32> = 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));
 

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.