Skip to content
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

Put font data into Arc to reduce memory consumption #5276

Merged

Conversation

StarStarJ
Copy link
Contributor

@StarStarJ StarStarJ commented Oct 16, 2024

egui never accesses the FontDefinitions' member fields mutably, except in fonts_tweak_ui where it cloned the FontDefinitions object anyway.

This patch reduces system memory consumption for shared font definitions.
And also removes some overhead from copying (e.g. for the per pixel_per_points font atlas)

Also it allows to keep a copy of the font definitions outside of egui.

In my App that uses international fonts:
Before:
image

New:
image

Note: If Arc is not wanted, then it could ofc be abstracted away.

I know this is quite a breaking change API wise, but would like to hear your opinion.

@StarStarJ
Copy link
Contributor Author

E.g. it keeps a copy in ContextImpl and copies it into the fonts per pixels_per_point:

self.font_definitions.clone(),

from:

let fonts = self
.fonts
.entry(pixels_per_point.into())
.or_insert_with(|| {
#[cfg(feature = "log")]
log::trace!("Creating new Fonts for pixels_per_point={pixels_per_point}");
is_new = true;
crate::profile_scope!("Fonts::new");
Fonts::new(
pixels_per_point,
max_texture_side,
self.font_definitions.clone(),
)
});

Copy link

Preview available at https://egui-pr-preview.github.io/pr/5276-pr_reduce_memory_usage_font_definitions
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@emilk
Copy link
Owner

emilk commented Oct 23, 2024

Let's wait until after #5228 is merged

@StarStarJ StarStarJ force-pushed the pr_reduce_memory_usage_font_definitions branch 3 times, most recently from 26d52dc to e624075 Compare October 26, 2024 13:59
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a simpler and smaller change would be

@@ -243,7 +243,7 @@ pub struct FontDefinitions {
     /// List of font names and their definitions.
     ///
     /// `epaint` has built-in-default for these, but you can override them if you like.
-    pub font_data: BTreeMap<String, FontData>,
+    pub font_data: BTreeMap<String, Arc<FontData>>,

@StarStarJ StarStarJ force-pushed the pr_reduce_memory_usage_font_definitions branch from e624075 to 18be843 Compare October 29, 2024 14:07
@StarStarJ StarStarJ changed the title Reduce the amount of FontDefinitions cloning, wrap it in Arc Put font data into Arc to reduce memory consumption Oct 29, 2024
@StarStarJ
Copy link
Contributor Author

I think a simpler and smaller change would be

@@ -243,7 +243,7 @@ pub struct FontDefinitions {
     /// List of font names and their definitions.
     ///
     /// `epaint` has built-in-default for these, but you can override them if you like.
-    pub font_data: BTreeMap<String, FontData>,
+    pub font_data: BTreeMap<String, Arc<FontData>>,

Sounds good aswell

@StarStarJ StarStarJ force-pushed the pr_reduce_memory_usage_font_definitions branch 2 times, most recently from 753d87c to 522b1ad Compare October 29, 2024 14:20
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better!

crates/egui/src/context.rs Outdated Show resolved Hide resolved
@emilk emilk added performance Lower CPU/GPU usage (optimize) egui epaint labels Oct 29, 2024
And additionally make cloning `FontDefinitions` cheaper.
@StarStarJ StarStarJ force-pushed the pr_reduce_memory_usage_font_definitions branch from 522b1ad to bef08ec Compare October 29, 2024 21:57
@emilk emilk merged commit 3f5cd74 into emilk:master Nov 1, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui epaint performance Lower CPU/GPU usage (optimize)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants