-
-
Notifications
You must be signed in to change notification settings - Fork 380
fix(perf): Remove FontSources.masks as they were consuming large amounts of memory even when no font sources were set #2519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
65123a6
b281ec1
62ee29a
0945a7c
57c66d5
e6c00ac
59ee365
40f583b
4ce28ad
e40ab2a
546d4a8
a9e72e4
a402e31
c59ff5b
b2768e3
1b7e250
95f6c9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,13 +16,12 @@ | |||||||||||||
| //! let font_data = sources.get_font_range("Arial,Helvetica", 0, 255).unwrap(); | ||||||||||||||
| //! ``` | ||||||||||||||
|
|
||||||||||||||
| use std::collections::HashMap; | ||||||||||||||
| use std::collections::{BTreeSet, HashMap}; | ||||||||||||||
| use std::ffi::OsStr; | ||||||||||||||
| use std::fmt::Debug; | ||||||||||||||
| use std::path::PathBuf; | ||||||||||||||
| use std::sync::LazyLock; | ||||||||||||||
| use std::sync::{Arc, LazyLock}; | ||||||||||||||
|
|
||||||||||||||
| use bit_set::BitSet; | ||||||||||||||
| use dashmap::{DashMap, Entry}; | ||||||||||||||
| use itertools::Itertools as _; | ||||||||||||||
| use pbf_font_tools::freetype::{Face, Library}; | ||||||||||||||
|
|
@@ -53,7 +52,7 @@ const RADIUS: usize = 8; | |||||||||||||
| const CUTOFF: f64 = 0.25_f64; | ||||||||||||||
| /// Maximum Unicode codepoint range ID. | ||||||||||||||
| /// | ||||||||||||||
| /// Each range is 256 codepoints long, so the highest range ID is 0xFFFF / 256 = 255. | ||||||||||||||
| /// Each range is 256 codepoints long, so the highest range ID is 0x10FFFF / 256 = 4351. | ||||||||||||||
| const MAX_UNICODE_CP_RANGE_ID: usize = MAX_UNICODE_CP / CP_RANGE_SIZE; | ||||||||||||||
|
|
||||||||||||||
| mod error; | ||||||||||||||
|
|
@@ -63,36 +62,37 @@ mod cache; | |||||||||||||
| pub use cache::{FontCache, NO_FONT_CACHE, OptFontCache}; | ||||||||||||||
|
|
||||||||||||||
| /// Glyph information: (codepoints, count, ranges, first, last). | ||||||||||||||
| type GetGlyphInfo = (BitSet, usize, Vec<(usize, usize)>, usize, usize); | ||||||||||||||
| type GetGlyphInfo = (BTreeSet<usize>, usize, Vec<(usize, usize)>, usize, usize); | ||||||||||||||
|
|
||||||||||||||
| /// Extracts available codepoints from a font face. | ||||||||||||||
| /// | ||||||||||||||
| /// Returns `None` if the font contains no usable glyphs. | ||||||||||||||
| fn get_available_codepoints(face: &mut Face) -> Option<GetGlyphInfo> { | ||||||||||||||
| let mut codepoints = BitSet::with_capacity(MAX_UNICODE_CP); | ||||||||||||||
| let mut codepoints = BTreeSet::new(); | ||||||||||||||
| let mut spans = Vec::new(); | ||||||||||||||
| let mut first: Option<usize> = None; | ||||||||||||||
| let mut count = 0; | ||||||||||||||
| let mut last = 0; | ||||||||||||||
|
|
||||||||||||||
| for cp in 0..=MAX_UNICODE_CP { | ||||||||||||||
| if face.get_char_index(cp).is_ok() { | ||||||||||||||
| codepoints.insert(cp); | ||||||||||||||
| count += 1; | ||||||||||||||
| if first.is_none() { | ||||||||||||||
| for (cp, _) in face.chars() { | ||||||||||||||
| codepoints.insert(cp); | ||||||||||||||
| if let Some(start) = first { | ||||||||||||||
| if cp != last + 1 { | ||||||||||||||
| spans.push((start, last)); | ||||||||||||||
| first = Some(cp); | ||||||||||||||
| } | ||||||||||||||
| } else if let Some(start) = first { | ||||||||||||||
| spans.push((start, cp - 1)); | ||||||||||||||
| first = None; | ||||||||||||||
| } else { | ||||||||||||||
| first = Some(cp); | ||||||||||||||
| } | ||||||||||||||
| last = cp; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if count == 0 { | ||||||||||||||
| None | ||||||||||||||
| } else { | ||||||||||||||
| if let Some(first) = first { | ||||||||||||||
| spans.push((first, last)); | ||||||||||||||
| let count = usize::try_from(face.num_glyphs()).unwrap(); | ||||||||||||||
| let start = spans[0].0; | ||||||||||||||
| let end = spans[spans.len() - 1].1; | ||||||||||||||
| Some((codepoints, count, spans, start, end)) | ||||||||||||||
| Some((codepoints, count, spans, start, last)) | ||||||||||||||
| } else { | ||||||||||||||
| None | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -123,25 +123,25 @@ pub struct FontSources { | |||||||||||||
| /// Map of font name to font source data. | ||||||||||||||
| fonts: DashMap<String, FontSource>, | ||||||||||||||
| /// Pre-computed bitmasks for each 256-character Unicode range. | ||||||||||||||
| masks: Vec<BitSet>, | ||||||||||||||
| masks: Arc<Vec<BTreeSet<usize>>>, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| impl Default for FontSources { | ||||||||||||||
| fn default() -> Self { | ||||||||||||||
| let mut masks = Vec::with_capacity(MAX_UNICODE_CP_RANGE_ID + 1); | ||||||||||||||
|
|
||||||||||||||
| let mut bs = BitSet::with_capacity(CP_RANGE_SIZE); | ||||||||||||||
| let mut set = BTreeSet::new(); | ||||||||||||||
| for v in 0..=MAX_UNICODE_CP { | ||||||||||||||
| bs.insert(v); | ||||||||||||||
| if v % CP_RANGE_SIZE == (CP_RANGE_SIZE - 1) { | ||||||||||||||
| masks.push(bs); | ||||||||||||||
| bs = BitSet::with_capacity(CP_RANGE_SIZE); | ||||||||||||||
| set.insert(v); | ||||||||||||||
| if (v % CP_RANGE_SIZE) == (CP_RANGE_SIZE - 1) { | ||||||||||||||
| masks.push(set); | ||||||||||||||
| set = BTreeSet::new(); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| Self { | ||||||||||||||
| fonts: DashMap::new(), | ||||||||||||||
| masks, | ||||||||||||||
| masks: Arc::new(masks), | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -181,19 +181,17 @@ impl FontSources { | |||||||||||||
| return Err(FontError::InvalidFontRange(start, end)); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| let mut needed = self.masks[(start as usize) / CP_RANGE_SIZE].clone(); | ||||||||||||||
| let needed: &BTreeSet<usize> = &self.masks[(start as usize) / CP_RANGE_SIZE]; | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nyurik Why did you use
Suggested change
In the testing I have done, this works fairly well, but there must be a reason why this is done this way that I am not getting..
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tbh, it has been a long time since i did it, so no clue :)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can imagine BitSet would work well for a small range like 0-255 but if you support the whole unicode range you need a lot of memory per bitset. |
||||||||||||||
| let fonts = ids | ||||||||||||||
| .split(',') | ||||||||||||||
| .filter_map(|id| match self.fonts.get(id) { | ||||||||||||||
| None => Some(Err(FontError::FontNotFound(id.to_string()))), | ||||||||||||||
| Some(v) => { | ||||||||||||||
| let mut ds = needed.clone(); | ||||||||||||||
| ds.intersect_with(&v.codepoints); | ||||||||||||||
| if ds.is_empty() { | ||||||||||||||
| if needed.intersection(&v.codepoints).count() == 0 { | ||||||||||||||
| None | ||||||||||||||
| } else { | ||||||||||||||
| needed.difference_with(&v.codepoints); | ||||||||||||||
| Some(Ok((id, v, ds))) | ||||||||||||||
| let diff: Vec<usize> = v.codepoints.difference(needed).copied().collect(); | ||||||||||||||
| Some(Ok((id, v, diff))) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| }) | ||||||||||||||
|
|
@@ -225,7 +223,7 @@ impl FontSources { | |||||||||||||
| // and https://www.freetype.org/freetype2/docs/tutorial/step1.html for details. | ||||||||||||||
| face.set_char_size(0, CHAR_HEIGHT, 0, 0)?; | ||||||||||||||
|
|
||||||||||||||
| for cp in &ds { | ||||||||||||||
| for cp in ds { | ||||||||||||||
| let glyph = render_sdf_glyph(&face, cp as u32, BUFFER_SIZE, RADIUS, CUTOFF)?; | ||||||||||||||
| stack.glyphs.push(glyph); | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -247,7 +245,7 @@ pub struct FontSource { | |||||||||||||
| /// Face index within the font file (for .ttc collections). | ||||||||||||||
| face_index: isize, | ||||||||||||||
| /// Unicode codepoints this font supports. | ||||||||||||||
| codepoints: BitSet, | ||||||||||||||
| codepoints: BTreeSet<usize>, | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsure if there is a better way. so we need to know which if the 256 bits -> While we curently store this as one very big
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noticed that only u32 is needed for the index, that is another halfing of that memory usage. This also stacks with the improvement above.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah there was a mix of usize and u32 around so definitely could standardise on u32 and avoid a bunch of casting
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with using a BitSet256 is you need to range it from 0-255 but the codepoints are 0-0x10FFFF so you either need to translate the requested codepoints back to 0-255 before masking (losing the performance of a bitset) or have one GIANT BitSet for each range. If you check the size of each BitSet before you'll see some are as big as 0x10FFFF (depending on the largest inserted value). |
||||||||||||||
| /// Font metadata for the catalog. | ||||||||||||||
| catalog_entry: CatalogFontEntry, | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -383,14 +381,13 @@ mod tests { | |||||||||||||
| // U+3320: SQUARE SANTIIMU, U+1F60A: SMILING FACE WITH SMILING EYES | ||||||||||||||
| for codepoint in [0x3320, 0x1f60a] { | ||||||||||||||
| let font_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) | ||||||||||||||
| .join(format!("src/resources/fonts/tests/u+{codepoint:x}.ttf")); | ||||||||||||||
| assert!(font_path.is_file()); // make sure the file path is correct | ||||||||||||||
|
|
||||||||||||||
| .join(format!("../tests/fixtures/fonts2/u+{codepoint:x}.ttf")); | ||||||||||||||
| assert!(font_path.is_file(), "{}", font_path.display()); | ||||||||||||||
| let mut face = lib.new_face(&font_path, 0).unwrap(); | ||||||||||||||
|
|
||||||||||||||
| let (_codepoints, count, _ranges, first, last) = | ||||||||||||||
| get_available_codepoints(&mut face).unwrap(); | ||||||||||||||
| assert_eq!(count, 1); | ||||||||||||||
| assert_eq!(count, 2); | ||||||||||||||
| assert_eq!(format!("U+{first:X}"), format!("U+{codepoint:X}")); | ||||||||||||||
| assert_eq!(format!("U+{last:X}"), format!("U+{codepoint:X}")); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the core issue is that although we allocate a BitSet::with_capacity(256) the value for v here might be 0x10FFFF causing a MASSIVE reallocation to fit values 0-0x10FFFF in the BitSet. From what I understand, the values must be contiguous without holes so each BitSet could be as large as (0x10FFFF / 32) (bits per u32) * 4 (bytes per u32) and then each of these is inserted for each range up to 4352.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my understanding is that each file would use a 136KB of RAM (
136*1024*8 = 1_114_112 = 0x11_0000codepoint presence bits) - not that much memory. If we follow the optimization I proposed below, most fonts won't even get that high, but it will keep the complexity to the minimal.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is less with each font source since it's only a single BitSet of 34816 u32 (139KB). The issue is with the pre-computed masks. Each range (of which there are 4352) carries a masking BitSet of up to
0x11_0000(34816 u32 [4 bytes] in the backing BitVec). Assuming every range is this size (inaccurately) we get 600MB (34816*4*4352bytes). More accurately we get masking BitSets with sizes growing from 8-34816 u32 and more likely around 300MB total. Prior to adjusting the max codepoint size we had smaller BitSets (8-2048 u32) and less of them 256 (0xFFFF/256). Resulting in at most ~2MB (2048*4*256bytes) for masks but likely less due to not all BitSets being 2048 size.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See images attached with debugger running on
mainbranch in thedefaultforFontSources: