[Impeller] cache glyph atlas if contents are unchanged #36719
Conversation
| // the current atlas and reuse if possible. | ||
| // --------------------------------------------------------------------------- | ||
| if (last_atlas->GetType() == type) { | ||
| auto new_glyphs = last_atlas->FindNewGlyphs(font_glyph_pairs); |
There was a problem hiding this comment.
I'm not sure if this is actually good enough of a check. If glyphs are drawn at different sizes I think we'd need to account for that ... right? But based on my reading of the code its only one FontGlyphPair mapped to each rect, so a different size glyph has to be a unique font glyph pair ... right?
There was a problem hiding this comment.
If the glyph size changes the rect won't match.
|
FYI @dnfield |
|
I ran some benchmarks on the flutter gallery using a newer iPhone. This is a tiny biy (0.5 -1 ms) faster, but we already know that isn't a particularly text heavy app. I'll have to try again with customer chalk when I'm back next week |
|
Playing around with the gallery app, I can see that on an older iPhone model (6? 7?), It can take 3-4ms to create a glyph atlas. Caching it means that the raster times for frames where the atlas is fully cached are measurably faster. This may or may not show up in synthetic benchmarks based on how quickly one "dwells" at a particular page in the app. |
| // --------------------------------------------------------------------------- | ||
| if (last_atlas->GetType() == type && | ||
| last_atlas->HasSamePairs(font_glyph_pairs)) { | ||
| return last_atlas; |
There was a problem hiding this comment.
We should file an issue to explore copying out the glyphs we still need. Or maybe just copy the whole atlas if it's small enough.
There was a problem hiding this comment.
I was thinking something like, if you need a small number of additional glyphs and they fit on the current atlas, just append them and re-upload. Even if we resized the atlas, reusing and blitting the old atlas instead of each character would be a good idea too
There was a problem hiding this comment.
We'd either have to keep the rect packer around or store the taken/free area somehow.
There was a problem hiding this comment.
Yeah, this probably deserves a doc exploring the options we have here
There was a problem hiding this comment.
I explored this a bit while I was out. There are a lot of potential caching options available to us, but I think we should probably keep pulling on making the whole thing faster first before we invest too much effort into tuning this.
To that end, lets land the trivial case caching and circle back?
Jonah asks for another review pass.
Attempt to improve average case performance by not recreating the glyph atlas if the contents are identical