[read-fonts] optimize post glyph name iter (#1983)

Custom glyph names in the `post` table are stored as an array of indices which reference a position in sequence of length prefixed (Pascal) strings. The "naive" approach to iteration, calling `glyph_name` for each gid, is quadratic over number of glyphs and leads to poor performance in the general case and potentially multi-second delays in the pathological case (all indices point to last name).

FreeType and HarfBuzz both use a precomputed, heap allocated offset buffer to avoid the cost. Since we'd prefer to avoid allocations, this patch introduces a sparse offset "checkpoint" buffer (currently with 16 entries) that is filled lazily during iteration and limits the linear search space to `num_glyphs / 17` (the 0 offset is implicit). It also caches the index and offset for the last glyph which is used for the start of the linear scan for the next glyph if it is closer than the nearest checkpoint. This leads to very fast iteration when the indices are (mostly) monotonic.

Also updates skrifa to use this.

Benchmarks for "naive" vs new cached iterator with 8k glyphs):

| Benchmark | Naive (ms) | Cached (ms) | Speedup |
| :--- | :---: | :---: | :---: |
| **Monotonic** | 45.241 | 0.164 | **276×** |
| **Mostly Monotonic** | 43.016 | 0.177 | **243×** |
| **Pathological** | 92.672 | 0.170 | **545×** |

Increasing the glyph count to 20k shows the scaling behavior:

| Benchmark | Naive (ms) | Cached (ms) | Speedup |
| :--- | :---: | :---: | :---: |
| **Monotonic** | 257.760 | 0.404 | **638×** |
| **Mostly Monotonic** | 260.030 | 0.501 | **519×** |
| **Pathological** | 530.350 | 0.413 | **1,285×** |

Note that the worst-case for the two iterators is now different. The cached iterator easily handles the "all indices point to last" structure due to the monotonic optimization. The absolute worst-case timing is triggered by a structure where the first index points to the last glyph, while remaining indices alternate between the ends of the first and second buckets. This layout forces repeated scans across an entire bucket width.

Comparing worst-case performance at 65k glyphs (all indices point to last for naive and the above layout for cached):

| Benchmark | Naive (ms) | Cached (ms) | Speedup |
| :--- | :---: | :---: | :---: |
| **Worst Case** | 6,184.20 | 329.82 | **18.8×** |

internal bug ref: b/537782685

Author: dfrg

font-test-data/src/lib.rs

@@ -158,6 +158,105 @@ pub mod closure {
 }
 
 pub mod post {
+    use crate::bebuffer::BeBuffer;
+
+    #[derive(Clone, Copy, Debug, PartialEq, Eq)]
+    pub enum GlyphNameOrder {
+        /// Glyph name indices in strictly increasing order.
+        Monotonic,
+        /// Glyph name indices in mostly increasing order, with some back
+        /// references to previous names.
+        ///
+        /// This likely matches most sane font data with duplicate glyph names.
+        MostlyMonotonicWithBackrefs,
+        /// All glyph name indices point to the last glyph name.
+        ///
+        /// This is the patholical case, requiring a full scan for each glyph
+        /// during iteration.
+        AllPointToLast,
+    }
+
+    /// Build a synthetic post v2 table with custom names for each glyph.
+    ///
+    /// Returns the table bytes and the generated glyph names in glyph id order.
+    pub fn v2_with_varied_glyph_names(
+        num_glyphs: u16,
+        base_name_len: u8,
+        order: GlyphNameOrder,
+    ) -> (Vec<u8>, Vec<String>) {
+        let custom_start = 258u16;
+        let mut buf = BeBuffer::new()
+            .push(0x0002_0000u32) // version 2.0
+            .push(0u32) // italicAngle
+            .push(0i16) // underlinePosition
+            .push(0i16) // underlineThickness
+            .push(0u32) // isFixedPitch
+            .push(0u32) // minMemType42
+            .push(0u32) // maxMemType42
+            .push(0u32) // minMemType1
+            .push(0u32) // maxMemType1
+            .push(num_glyphs);
+        let mapped_indices = mapped_custom_indices(num_glyphs as usize, order);
+        for mapped_idx in mapped_indices.iter().copied() {
+            let mapped_idx_u16 = u16::try_from(mapped_idx).unwrap();
+            buf = buf.push(custom_start.saturating_add(mapped_idx_u16));
+        }
+        let mut custom_names = Vec::with_capacity(num_glyphs as usize);
+        for custom_idx in 0..num_glyphs as usize {
+            let name = varied_name(custom_idx, base_name_len);
+            let len = u8::try_from(name.len()).unwrap();
+            buf = buf.push(len).extend(name.as_bytes().iter().copied());
+            custom_names.push(name);
+        }
+        let names = mapped_indices
+            .iter()
+            .map(|idx| custom_names[*idx].clone())
+            .collect();
+        (buf.data().to_vec(), names)
+    }
+
+    fn mapped_custom_indices(num_glyphs: usize, order: GlyphNameOrder) -> Vec<usize> {
+        match order {
+            GlyphNameOrder::Monotonic => (0..num_glyphs).collect(),
+            GlyphNameOrder::MostlyMonotonicWithBackrefs => (0..num_glyphs)
+                .map(|gid| {
+                    if gid > 0 && gid % 97 == 0 {
+                        gid - 1
+                    } else if gid > 3 && gid % 251 == 0 {
+                        gid - 3
+                    } else if gid > 7 && gid % 509 == 0 {
+                        gid - 7
+                    } else {
+                        gid
+                    }
+                })
+                .collect(),
+            GlyphNameOrder::AllPointToLast => {
+                if num_glyphs == 0 {
+                    Vec::new()
+                } else {
+                    vec![num_glyphs - 1; num_glyphs]
+                }
+            }
+        }
+    }
+
+    fn varied_name(custom_idx: usize, base_name_len: u8) -> String {
+        let base_name_len = usize::from(base_name_len.clamp(8, 220));
+        let len = (base_name_len + (custom_idx % 27)).min(255);
+        let mut name = format!("glyph_{custom_idx:05}_");
+        let pattern = match custom_idx % 4 {
+            0 => "abcdefghijklmnopqrstuvwxyz0123456789",
+            1 => "zyxwvutsrqponmlkjihgfedcba9876543210",
+            2 => "aabccddeeffgghhiijjkkllmmnnooppqqrrss",
+            _ => "n0nlinear_name_pattern_block_",
+        };
+        while name.len() < len {
+            name.push_str(pattern);
+        }
+        name.truncate(len);
+        name
+    }
 
     #[rustfmt::skip]
     pub static SIMPLE: &[u8] = &[

read-fonts/Cargo.toml

@@ -65,3 +65,7 @@ harness = false
 [[bench]]
 name = "table_lookup"
 harness = false
+
+[[bench]]
+name = "glyph_names"
+harness = false

read-fonts/benches/glyph_names.rs

@@ -0,0 +1,66 @@
+//! Benchmark that compares naive iteration over glyph names with optimized
+//! iteration using `Post::glyph_names`.
+
+use core::hint::black_box;
+use criterion::{criterion_group, criterion_main, Criterion};
+use font_test_data::post::GlyphNameOrder;
+use read_fonts::{tables::post::Post, FontRead};
+
+const NUM_GLYPHS: u16 = 8_000;
+const BASE_NAME_LEN: u8 = 20;
+
+fn mode_name(mode: GlyphNameOrder) -> &'static str {
+    match mode {
+        GlyphNameOrder::Monotonic => "monotonic",
+        GlyphNameOrder::MostlyMonotonicWithBackrefs => "mostly_monotonic_with_backrefs",
+        GlyphNameOrder::AllPointToLast => "all_point_to_last",
+    }
+}
+
+/// Iterate glyph names by looping over indices and calling `Post::glyph_name`.
+pub fn glyph_names_iter_naive(c: &mut Criterion) {
+    let modes = [
+        GlyphNameOrder::Monotonic,
+        GlyphNameOrder::MostlyMonotonicWithBackrefs,
+        GlyphNameOrder::AllPointToLast,
+    ];
+    for mode in modes {
+        let post_data =
+            font_test_data::post::v2_with_varied_glyph_names(NUM_GLYPHS, BASE_NAME_LEN, mode).0;
+        let post = Post::read(post_data.as_slice().into()).unwrap();
+        let bench_name = format!("glyph_names_iter_naive_synth_post_v2/{}", mode_name(mode));
+        c.bench_function(&bench_name, |b| {
+            b.iter(|| {
+                let total_len: usize = (0..post.num_glyphs().unwrap())
+                    .filter_map(|gid| post.glyph_name(gid.into()))
+                    .map(str::len)
+                    .sum();
+                black_box(total_len)
+            });
+        });
+    }
+}
+
+/// Use optimized `Post::glyph_names` iterator to iterate glyph names.
+pub fn glyph_names_iter(c: &mut Criterion) {
+    let modes = [
+        GlyphNameOrder::Monotonic,
+        GlyphNameOrder::MostlyMonotonicWithBackrefs,
+        GlyphNameOrder::AllPointToLast,
+    ];
+    for mode in modes {
+        let post_data =
+            font_test_data::post::v2_with_varied_glyph_names(NUM_GLYPHS, BASE_NAME_LEN, mode).0;
+        let post = Post::read(post_data.as_slice().into()).unwrap();
+        let bench_name = format!("glyph_names_iter_cached_synth_post_v2/{}", mode_name(mode));
+        c.bench_function(&bench_name, |b| {
+            b.iter(|| {
+                let total_len: usize = post.glyph_names().map(|(_, name)| name.len()).sum();
+                black_box(total_len)
+            });
+        });
+    }
+}
+
+criterion_group!(benches, glyph_names_iter_naive, glyph_names_iter);
+criterion_main!(benches);

read-fonts/src/tables/post.rs

@@ -13,7 +13,13 @@ impl<'a> Post<'a> {
         }
     }
 
-    pub fn glyph_name(&self, glyph_id: GlyphId16) -> Option<&str> {
+    /// Returns the name for the given glyph.
+    ///
+    /// Note that this is a relatively expensive operation, as it may require
+    /// a linear scan through the string data to find the target name. If you
+    /// need to iterate over all glyph names or collect them into a map for
+    /// faster access, use [`Self::glyph_names`] instead.
+    pub fn glyph_name(&self, glyph_id: GlyphId16) -> Option<&'a str> {
         let glyph_id = glyph_id.to_u16() as usize;
         match self.version() {
             Version16Dot16::VERSION_1_0 => DEFAULT_GLYPH_NAMES.get(glyph_id).copied(),
@@ -29,6 +35,24 @@ impl<'a> Post<'a> {
         }
     }
 
+    /// Return an iterator over the glyph names in this table.
+    pub fn glyph_names(&self) -> GlyphNames<'a> {
+        let num_names = self.num_names() as u32;
+        let kind = match self.version() {
+            Version16Dot16::VERSION_1_0 => GlyphNameIterKind::V1(self.clone(), 0),
+            Version16Dot16::VERSION_2_0 => GlyphNameIterKind::V2 {
+                post: self.clone(),
+                idx: 0,
+                checkpoint_stride: (num_names as usize).div_ceil(NUM_CHECKPOINTS + 1).max(1),
+                checkpoints: [UNSET_CHECKPOINT; NUM_CHECKPOINTS],
+                last_actual_idx: None,
+                last_offset: 0,
+            },
+            _ => GlyphNameIterKind::None,
+        };
+        GlyphNames { num_names, kind }
+    }
+
     //FIXME: how do we want to traverse this? I want to stop needing to
     // add special cases for things...
     #[cfg(feature = "experimental_traverse")]
@@ -88,6 +112,126 @@ impl VarSize for PString<'_> {
     type Size = u8;
 }
 
+const NUM_CHECKPOINTS: usize = 16;
+const UNSET_CHECKPOINT: u32 = u32::MAX;
+
+/// Iterator over the glyph names in a post table.
+#[derive(Clone)]
+pub struct GlyphNames<'a> {
+    num_names: u32,
+    kind: GlyphNameIterKind<'a>,
+}
+
+#[derive(Clone)]
+enum GlyphNameIterKind<'a> {
+    None,
+    V1(Post<'a>, u32),
+    V2 {
+        post: Post<'a>,
+        idx: u32,
+        // The number of indices between checkpoints
+        checkpoint_stride: usize,
+        // The offset of each checkpoint; checkpoint 0 is implicit
+        checkpoints: [u32; NUM_CHECKPOINTS],
+        // The last actual index and that was scanned, for monotonic fast path
+        last_actual_idx: Option<usize>,
+        // The offset associated with the last scanned index
+        last_offset: usize,
+    },
+}
+
+impl<'a> Iterator for GlyphNames<'a> {
+    type Item = (GlyphId, &'a str);
+
+    fn next(&mut self) -> Option<Self::Item> {
+        match &mut self.kind {
+            GlyphNameIterKind::None => None,
+            GlyphNameIterKind::V1(post, idx) => {
+                if *idx >= self.num_names {
+                    return None;
+                }
+                let gid = GlyphId16::new(*idx as u16);
+                let name = post.glyph_name(gid)?;
+                *idx += 1;
+                Some((gid.into(), name))
+            }
+            GlyphNameIterKind::V2 {
+                post,
+                idx,
+                checkpoint_stride,
+                checkpoints,
+                last_actual_idx,
+                last_offset,
+            } => {
+                if *idx >= self.num_names {
+                    return None;
+                }
+                let stride = *checkpoint_stride;
+                let gid = GlyphId16::new(*idx as u16);
+                let mut actual_idx = post.glyph_name_index()?.get(*idx as usize)?.get() as usize;
+                let name = if actual_idx < DEFAULT_GLYPH_NAMES.len() {
+                    DEFAULT_GLYPH_NAMES.get(actual_idx).copied()?
+                } else {
+                    actual_idx -= DEFAULT_GLYPH_NAMES.len();
+                    let string_data = post.data.slice(post.string_data_byte_range())?;
+                    // Checkpoint 0 is implicit and always at offset 0; the
+                    // array stores logical checkpoints 1..=NUM_CHECKPOINTS.
+                    let target_slot = (actual_idx / stride).min(NUM_CHECKPOINTS);
+                    // Find the the starting location for our scan
+                    let (mut scan_idx, mut offset) = {
+                        // Search backward from the target slot to find the
+                        // nearest checkpoint that has been set
+                        let mut slot = target_slot;
+                        while slot > 0 && checkpoints[slot - 1] == UNSET_CHECKPOINT {
+                            slot -= 1;
+                        }
+                        if slot == 0 {
+                            // Fallback to implicit checkpoint 0
+                            (0, 0)
+                        } else {
+                            // Otherwise, start scanning from the nearest
+                            // checkpoint
+                            (slot * stride, checkpoints[slot - 1] as usize)
+                        }
+                    };
+                    // See if we can use the monotonic fast path.
+                    if let Some(last_idx) = *last_actual_idx {
+                        // Start scanning from the last index if that provides
+                        // a smaller search space than the nearest checkpoint
+                        if last_idx <= actual_idx && last_idx > scan_idx {
+                            scan_idx = last_idx;
+                            offset = *last_offset;
+                        }
+                    }
+                    // Now do the linear scan over the string data
+                    while scan_idx < actual_idx {
+                        let item_len = PString::read_len_at(string_data, offset)?;
+                        offset = offset.checked_add(item_len)?;
+                        scan_idx += 1;
+                        // If this index is a checkpoint, record the offset
+                        // for future scans
+                        if scan_idx % stride == 0 {
+                            let slot = (scan_idx / stride).min(NUM_CHECKPOINTS);
+                            if slot > 0 {
+                                checkpoints[slot - 1] = u32::try_from(offset).ok()?;
+                            }
+                        }
+                    }
+                    if actual_idx % stride == 0 && target_slot > 0 {
+                        checkpoints[target_slot - 1] = u32::try_from(offset).ok()?;
+                    }
+                    // Record the last index and offset for future scans
+                    *last_actual_idx = Some(actual_idx);
+                    *last_offset = offset;
+                    PString::read(string_data.split_off(offset)?).ok()?.0
+                };
+                *idx += 1;
+                Some((gid.into(), name))
+            }
+        }
+    }
+}
+
 /// The 258 glyph names defined for Macintosh TrueType fonts
 #[rustfmt::skip]
 pub static DEFAULT_GLYPH_NAMES: [&str; 258] = [
@@ -206,4 +350,27 @@ mod tests {
         // Just don't panic
         assert_eq!(post.glyph_name(GlyphId16::new(0)), None);
     }
+
+    #[test]
+    fn glyph_names_matches_naive_on_varied_synthetic_v2_data() {
+        let num_glyphs = 2_000u16;
+        let orders = [
+            test_data::GlyphNameOrder::Monotonic,
+            test_data::GlyphNameOrder::MostlyMonotonicWithBackrefs,
+            test_data::GlyphNameOrder::AllPointToLast,
+        ];
+        for order in orders {
+            let (bytes, expected_custom_names) =
+                test_data::v2_with_varied_glyph_names(num_glyphs, 63, order);
+            let post = Post::read(bytes.as_slice().into()).unwrap();
+            let from_naive: Vec<_> = (0..num_glyphs)
+                .map(|gid| post.glyph_name(GlyphId16::new(gid)).unwrap())
+                .collect();
+            let from_iter: Vec<_> = post.glyph_names().map(|(_, name)| name).collect();
+            assert_eq!(from_iter, from_naive);
+            for (name, expected) in from_iter.iter().zip(expected_custom_names.iter()) {
+                assert_eq!(*name, expected.as_str());
+            }
+        }
+    }
 }