[Merged by Bors] - Limit FontAtlasSets

[xtr3m3nerd]

Objective

Fixes #5636
Summary: The FontAtlasSet caches generated font textures per font size. Since font size can be any arbitrary floating point number it is possible for the user to generate thousands of font texture inadvertently by changing the font size over time. This results in a memory leak as these generated font textures fill the available memory.

Solution

We limit the number of possible font sizes that we will cache and throw an error if the user attempts to generate more. This error encourages the user to use alternative, less performance intensive methods to accomplish the same goal. If the user requires more font sizes and the alternative solutions wont work there is now a TextSettings Resource that the user can set to configure this limit.

---

Changelog

The number of cached font sizes per font is now limited with a default limit of 100 font sizes per font. This limit is configurable via the new TextSettings struct.

[xtr3m3nerd]

I was modeling TextSettings around how settings are handled in other modules, however considering the management of FontAtlasSet's are layered fairly deep inside of a system the value has to be passed around a lot, which didnt feel right...

I was running a blank on a better way to handle this, so if there are any suggestions I would appreciate it.

[tigregalis]

While strictly speaking this could solve the unbounded memory usage (by bounding it), is the intention that we should support using many font sizes, or not support it?

If it is not meant to be supported (currently you error when there are too many), then 100 seems like an unnecessarily large default, perhaps 16?

If it is meant to be supported, then rather than erroring out, you could implement a LRU cache-like thing - pseudocode:

pub struct FontAtlasSet { 
    font_atlases: HashMap<FontSizeKey, Vec<FontAtlas>>, 
    queue: Vec<FontSizeKey>, // or some other type? VecDeque? IndexSet?
} 
// ...
    pub fn add_glyph_to_atlas(
        &mut self,
        texture_atlases: &mut Assets<TextureAtlas>,
        textures: &mut Assets<Image>,
        outlined_glyph: OutlinedGlyph,
        text_settings: &TextSettings,
    ) -> Result<GlyphAtlasInfo, TextError> {
        //  find the size in self.font_atlases
        //  if found then
        //    take the size out and move it to the front of the queue (most recently used)
        //  else (if not found then)
        //    push the size onto the front of the queue (most recently used)
        //  while self.queue.len() >= text_settings.max_font_atlases
        //    remove the item at the back of the queue (the least recently used):
        //      remove the size from self.recent
        //      remove the size from self.font_atlases
        //      remove those font atlases
        //  proceed as normal
        //  
        //  elsewhere, once a frame, sort the queue, so that any font sizes that aren't currently in use are moved to the back of the queue (least recently used)
        //
        //  somehow, somewhere, handle the case where you want to delete a font size that is actually in use
    }

[djeedai]

I think that either we want to fix the issue where floating point imprecision creates 2 copies and in that case we can simply round to say 1/16th of a pixel and be done, or we want to limit GPU resources and what makes sense is not the number of fonts or glyphs but the total cache RAM usage in GPU memory (so, number of textures), in which case yes we might need an LRU cache.

[xtr3m3nerd]

So the reason why I think we should have it error out vs an LRU cache is that there is a high performance cost every time you add_glyph_to_atlas since there is a texture that needs to be generated. That is why we cache it in the first place so we only have to do that work once and in 99% of use cases this is just fine.

If we implement an LRU cache that does solve the memory usage issue, but it encourages the user to use font size for dynamically resizing the text they want to display. This will mean every frame the engine is generating these glyph textures which then gets thrown out the next frame negating the benefit of the hashmap in the first place (in that specific use case). In that case I believe we should be encouraging users to modify the scale of the text object rather than using font-size since the outputted results will be handled by the graphics apis and have a very low performance cost, while achieving very similar results.

[xtr3m3nerd]

Just to add onto that. The user generally doesn't know what is going on in the engine under the hood. If the user was aware that there is a performance cost to dynamically changing font-size they would perhaps consider alternatives. However if they aren't aware of this cost they will likely write slow code without realizing it and then wonder why the engine is so slow at handling fonts. The error serves as a way for the engine to let the user know they are using behavior in a way that is unintended and has cost implications(both memory and cpu usage). Perhaps we allow the user to configure to use the LRU cache, but I believe by default we should still error out since it will best educate the user on what to expect from the engine.

[mockersf]

Is this still needed if the font sized is switched to an int as proposed in #5636 (comment)?

[xtr3m3nerd]

I suppose not, there is still some risk of the user abusing the system, but its damage is much more limited. Ill go ahead and make the change and see if there are any downsides that come up in testing.

Coding etiquette question. Would it be better to revert my changes and make the new purposed changes in this PR or to use a separate PR?

[xtr3m3nerd]

So I was looking at the implication of switch the font size from f32 to u32

First off it is much more involved than initially thought.

Font size is not something that is just set by the user, it is also calculated depending on screen scale and the individual scales of the text sections. This means there are many places that would need to be updated no matter how you slice it.

Additionally while this change does reduce the likely hood of a memory leak issue it doesn't outright prevent it and still allows the user to write some fairly memory hungry code with a high upfront cpu costs.

Lastly after checking, font_size being an int is not as universal as we thought. Unity definitely stores the font as a float and supports fractional font sizes. Godot does use int and I am unsure about Unreal because I don't have it installed on my system.

All that considering I no longer think changing the font size from a f32 to u32 is the right way to go to resolve this issue.

I have implemented the LRU Cache proposed here #5708 (comment) and added a setting to allow the user to choose.

Here are the reasons why I think we should use this change over the f32 to u32 fix:

• This solves the underlying issue
• Educates the user on what the system is doing when they use it in an unintended manner
• Still give the user the flexibility to use the system in the unintended manner while also addressing the memory leak
• Does not introduce breaking changes.
• Still supports the screen scale
• Supports fractional font_sizes
• Change in scope is restricted to the text system.

[IceSentry]

Couple of nitpicks, but overall the implementation looks good

[alice-i-cecile]

@xtr3m3nerd please feel free to resolve conversations once you've addressed their concerns; it makes it much easier for reviewers to follow :)

[alice-i-cecile]

I'm liking the rotating buffer solution a lot better, and your reasoning against swapping to integer-like font sizes is sensible.

Some comments on the representation of types and more graceful error handling for you :)

[alice-i-cecile]

IMO this setting plus the one above should be refactored to a Option<NonZeroUsize>.

This reduces the number of fields to reason about, and ensures that users handle the "there is no limit" case correctly. Make invalid states unrepresentable and all that :)

[xtr3m3nerd]

So on this point I disagree that we should merge this into a single field, the reason for that is that in this implementation we do not and should not support a "there is no limit case".
The two use cases being supported here are:

1. Panicking if the user surpasses the max_font_atlases limit (allow_dynamic_font_size == false | default behavior)
2. Use a rotating buffer of the size max_font_atlases if the limit is exceeded (allow_dynamic_font_size == true | this behavior must be set by the user since it has a high performance cost associated with it)

In each case both fields are necessary to determine the behavior and the size of the buffer. I do agree that max_font_atlases should be changed to NonZeroUsize since there is no use case where we would want it to be 0. I have made a change to reflect that.

We should not support a "there is no limit" because ultimately there would leave no protection for the user from the memory leak and with these two settings they should be able to reach any desired behavior while still providing safeguards.

[alice-i-cecile]

Ah, I see! Okay, please feel free to resolve this comment then.

[alice-i-cecile]

This doesn't feel like it needs to be a panic; aren't we just retiring the latest atlas when the max number is exceeded? We can recover from handling any number of text atlases at the cost of recomputation.

[xtr3m3nerd]

So as mentioned in the comment above this change is supporting two different use cases:

1. Panicking if the user surpasses the max_font_atlases limit (allow_dynamic_font_size == false | default behavior)
2. Use a rotating buffer of the size max_font_atlases if the limit is exceeded (allow_dynamic_font_size == true | this behavior must be set by the user since it has a high performance cost associated with it)

The panic in the default behavior is important because it is notifying the user that the font system is being used in a way that it was not designed to. The point of setting allow_dynamic_font_size=true is to allow the user to bypass this error in which case it will start using the rotating buffer. If the user does this in a use case like the one in which I discovered this memory leak they would be experiencing a large performance drop since every frame a new text font atlas would be generated and then thrown out a couple frames later. In this situation the cache is effectively pointless since the font needed is generated each frame, but again this comes at a noticeable performance cost.

With the panic we are letting the user know they are doing something dangerous and perhaps should rethink how they are approaching the problem. If they absolutely need to generate an endless amount of fonts and are aware of the cost associated with doing that then they can set allow_dynamic_font_size=true, but I do not think in any way this should be the default behavior because that would just set up a user who has no understanding of how the font system works under the hood confused as to why their code is running so slow just to change font sizes.

To me the performance hit was visually noticeable with an immediate drop in frame rate. If it would help I can do some performance tests to get some hard numbers on this cost. This is the primary reason why I do not suggest having the rotating buffer be the default behavior. With the panic we give the user an opportunity to educate themselves on the implications of using the system in this manner before they choose to make the switch.

[alice-i-cecile]

Ditto on this panic.

[xtr3m3nerd]

See response on other panic

[alice-i-cecile]

You've convinced me; I think this is a good next step forward.

bors r+

[bors]

Build failed (retrying...):

• build-and-install-on-iOS

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12