Repository navigation
Started caching text shadows by content. - #190681
Conversation
7463502 to
8a95c63
Compare
40b4ea0 to
24d68b0
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-attribute fingerprinting mechanism (TextFrameFingerprint) for text frames in Impeller's TextShadowCache to prevent key collisions without storing heap pointers, along with corresponding unit tests. The review feedback suggests improving the robustness of ComputeTextFrameFingerprint when handling empty text runs, simplifying the conditional logic in Canvas::AttemptBlurredTextOptimization, and optimizing the memory footprint of TextFrameFingerprint by using uint32_t instead of size_t for run and glyph counts.
48d345d to
8a2e544
Compare
issue flutter#190395 ## test results on macos Metric | Stock Engine (host_profile) | Patched Engine (text-shadow-cache-by-content) | Improvement --------------------------------------------|--------------------------------------------|-----------------------------------------------|------------------------------------------- Average Raster Time | 6.82 ms | 1.11 ms | ~6.1x faster 90th Percentile Raster Time | 8.66 ms | 1.72 ms | ~5.0x faster 99th Percentile Raster Time | 9.73 ms | 2.53 ms | ~3.8x faster Worst Frame Raster Time | 10.14 ms | 2.67 ms | ~3.8x faster The patch is from flutter#190681 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
|
| uint32_t first_glyph_id = 0; | ||
| uint32_t last_glyph_id = 0; |
There was a problem hiding this comment.
Why not just store the Glyph itself instead of packing it into a uint32?
There was a problem hiding this comment.
The thought is that the fingerprint is kind of like an expanded hash value. So I kept the datatypes as primitives so it can be seen a such.
(See also other comment about managing dependencies)
There was a problem hiding this comment.
How costly is the act of comparing the contents when we find a hit in the cache? It looks like this Fingerprint is trying to avoid a comparison, but at the cost of having to recompute the fingerprint on every render operation anyway (which also does more work than a comparison would with the hash equations). I would think that just adding hash and == to TextFrame would accomplish the same thing with nearly identical cost, wouldn't it?
| int64_t identifier = maybe_glyph.has_value() | ||
| ? maybe_glyph.value().index | ||
| : reinterpret_cast<int64_t>(text_frame.get()); | ||
| TextFrameFingerprint fingerprint = |
There was a problem hiding this comment.
It seems kind of weird to me to have ComputeTextFrameFingerprint as a function in canvas.cc, and to make canvas.cc responsible for calculating the fingerprint and cache key to use for TextShadowCache. It doesn't seem like something that should happen at the canvas layer of abstraction.
Every client of TextShadowCache must need this exact same logic. So I think this logic should be part of TextShadowCache. What about instead of Lookup() taking cache_key, it just takes a TextFrame, and Lookup/TextShadowCache calculates the fingerprint and cache key internally?
There was a problem hiding this comment.
Practically, there is only one client of TextShadowCache, the canvas. I think if that changes we'd naturally have to put this in a more accessible place. We never needed to make it visible because we test it with integration tests. Some people would consider putting code only used from one place into a shared compilation unit a premature optimization (but not putting it in a shared location can make it harder to find if someone needs it in the future).
There is an architectural benefit of keeping TextShadowCache independent from TextFrame. It's easier to understand and test in isolation the less dependencies something has. I think this is a clean break the way things stand right now and maintains the existing dependencies.
I don't think it's a huge deal either way, but it isn't a clear improvement to make the suggested change, it's a tradeoff.
There was a problem hiding this comment.
Some people would consider putting code only used from one place into a shared compilation unit a premature optimization
I don't think it's a matter of optimization, but it's more to do with keeping the code architecturally cleaner and more readable with encapsulation and separation of responsibilities.
There is an architectural benefit of keeping TextShadowCache independent from TextFrame
TextShadowCache is already implicitly very dependent on TextFrame, because TextFrameFingerprint and other fields in TextShadowCacheKey are implicitly tied to fields of TextFrame. And it specifically being a cache for the shadows of TextFrames also implicitly ties it to TextFrame. I think implicit dependencies are a code smell, and making them explicit makes code simpler and easier to follow.
If we really did want to keep TextShadowCache separate from TextFrame, I think the way to do that would be to make it a generic EntityCache that accepts an arbitrary hashable or templated key type. Then the client is fully responsible for creating the cache key, and there is no implicit dependency of the cache on knowing any details of the key or how it's created. But this is probably more complicated than what we need.
In my opinion I think it's a clear an improvement to make TextShadowCache explicitly use TextFrame, and not have canvas.cc create the cache key. But I don't feel super strongly, so if you prefer otherwise I'm not blocking on this.
There was a problem hiding this comment.
But I don't feel super strongly, so if you prefer otherwise I'm not blocking on this.
Yea, same. I just wanted to point out an alternative view to show it was something I thought about and potentially give another perspective (and for fun).
I don't think it's a matter of optimization, but it's more to do with keeping the code architecturally cleaner and more readable with encapsulation and separation of responsibilities.
One thing I'm not sure if you are considering is that ComputeTextFrameFingerprint is a free function in an anonymous namespace. There is nothing more encapsulated or separated from responsibilities because it doesn't have any state of its own and has a limited clear set of users. All the context and information it needs is in the function. It could be copy and pasted anywhere verbatim without change. It isn't a method on the class Canvas. When talking about free functions, it's the calculus is a bit different as opposed to methods.
There was a problem hiding this comment.
How expensive is computing the fingerprint? TextFrame is pretty Impeller specific so I think it would be appropriate to store the hash in the frame (perhaps lazily computed). Any other code that uses a TextFrame (are there any?) can either use it as computed for Canvas, or ignore it. I'm not sure we have needs for incompatible hashing or that a later hashing need would come up that would be incompatible with what we've already done here.
There was a problem hiding this comment.
Yea, I thought of that. It's O(n), should be fast. In the benchmarks the wins from the cache overshadow any overhead from not memoizing the fingerprint. I gave this PR the lightest touch to give us the result we wanted. Someone could measure memoizing the fingerprints. It's a cost we only pay with shadowed text so I didn't see it as high priority either.
There was a problem hiding this comment.
And also this is only when a blur is detected, so it is hiding behind an already expensive operation and not costing anything for the vast majority of text operations...
…#12453) Manual roll Flutter from 27b098811f3b to c2437523d308 (179 revisions) Manual roll requested by tarrinneal@google.com flutter/flutter@27b0988...c243752 2026-08-12 matt.boetger@gmail.com Enable Gradle CI cache on all test targets that require android_sdk (flutter/flutter#190723) 2026-08-12 bkonyi@google.com [analysis] Reland "Added initial implementation of the flutter_analyzer_plugin (#175679)" (flutter/flutter#191022) 2026-08-12 matt.boetger@gmail.com Switch testing to gradle bin distribution type instead of all (flutter/flutter#190738) 2026-08-12 matt.boetger@gmail.com Convert Mockito instances in Kotlin to Mockk (flutter/flutter#189884) 2026-08-12 chingjun@google.com Report individual test results to LUCI ResultDB (flutter/flutter#190254) 2026-08-12 victorsanniay@gmail.com Toggleable reaction duration respects overrides (flutter/flutter#190857) 2026-08-12 engine-flutter-autoroll@skia.org Roll Skia from e00dbd7448c4 to fee7272f5bc2 (1 revision) (flutter/flutter#191007) 2026-08-12 30870216+gaaclarke@users.noreply.github.com Started caching text shadows by content. (flutter/flutter#190681) 2026-08-12 30870216+gaaclarke@users.noreply.github.com Adds agent skill for spawning led tasks. (flutter/flutter#190937) 2026-08-12 engine-flutter-autoroll@skia.org Roll Packages from aaaf246 to 94485f1 (8 revisions) (flutter/flutter#191008) 2026-08-12 82978131+herdiyana256@users.noreply.github.com flutter_tools: validate plugin identifiers before generating registrant code (flutter/flutter#190462) 2026-08-12 engine-flutter-autoroll@skia.org Roll Skia from 112f36148949 to e00dbd7448c4 (3 revisions) (flutter/flutter#190993) 2026-08-12 engine-flutter-autoroll@skia.org Roll Skia from 7d366c802307 to 112f36148949 (3 revisions) (flutter/flutter#190983) 2026-08-12 okorohelijah@google.com remove bringup for flavors test (flutter/flutter#190940) 2026-08-12 engine-flutter-autoroll@skia.org Roll Skia from 1f10a20bdd61 to 7d366c802307 (2 revisions) (flutter/flutter#190980) 2026-08-12 116356835+AbdeMohlbi@users.noreply.github.com Remove `--no-sim-use-hardfp` flag (flutter/flutter#190790) 2026-08-12 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#190977) 2026-08-12 engine-flutter-autoroll@skia.org Roll Skia from 339bedab6766 to 1f10a20bdd61 (1 revision) (flutter/flutter#190975) 2026-08-12 victorsanniay@gmail.com RawTooltip respects AnimationStyle updates and reverseCurve (flutter/flutter#190889) 2026-08-12 chris@bracken.jp ci: Support --target_arch option in prepare_package.dart (flutter/flutter#190960) 2026-08-12 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from SFq4FVodIOQAS26Lr... to -uHuSGv3wt7QAlDwa... (flutter/flutter#190973) 2026-08-12 aam@google.com Removes building of ci/android_debug_x86 as nobody should be consuming it. (flutter/flutter#190951) 2026-08-12 30870216+gaaclarke@users.noreply.github.com Adds error about wimp_heavy not being implemented. (flutter/flutter#189945) 2026-08-11 robert.ancell@canonical.com Add clang, cmake, and ninja deps to Linux windowing_test (flutter/flutter#190119) 2026-08-11 269567208+reidbaker-agent@users.noreply.github.com [AGP 9.1.0 Migration #1] Add Android Gradle Plugin Public API migration documentation (flutter/flutter#190842) 2026-08-11 bkonyi@google.com [flutter_tools] Fix deadlock in debug adapters when process exits early (flutter/flutter#190931) 2026-08-11 bkonyi@google.com [tool] Define modular dependency injection containers and bootstrapper (flutter/flutter#190724) 2026-08-11 1961493+harryterkelsen@users.noreply.github.com [web] Unify MaskFilter and ColorFilter primitives across CanvasKit and Skwasm (flutter/flutter#190314) 2026-08-11 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#190958) 2026-08-11 33794642+FelixMittermeier@users.noreply.github.com [Impeller] Move image upload scheduling waits to GPU disable (flutter/flutter#190445) 2026-08-11 30870216+gaaclarke@users.noreply.github.com Started generating the windows platform for macrobenchmarks (flutter/flutter#190932) 2026-08-11 1961493+harryterkelsen@users.noreply.github.com [web] Unify ui.Vertices (flutter/flutter#190563) 2026-08-11 jhy03261997@gmail.com Fix accessibility_inspector service extensions map mutability (flutter/flutter#190888) 2026-08-11 47866232+chunhtai@users.noreply.github.com Add batch3 a11y_assessment for vpat (flutter/flutter#189042) 2026-08-11 15619084+vashworth@users.noreply.github.com Remove Xcode environment when building swift tools in Xcode pre-action (flutter/flutter#190848) 2026-08-11 mdebbar@google.com [tool] Add missing play element in web test index.html to fix warning (flutter/flutter#190675) 2026-08-11 kkmk1999@gmail.com Offload blocking work in ProcessTextPlugin to the background (flutter/flutter#189823) 2026-08-11 bkonyi@google.com [flutter_tools] Replace usages of package:dds/dap.dart with package:dap_adapters/dap_adapters.dart (flutter/flutter#190667) 2026-08-11 15619084+vashworth@users.noreply.github.com Always update swift package dependencies (flutter/flutter#190886) 2026-08-11 bkonyi@google.com [flutter_tools] Add --preset option to flutter test (flutter/flutter#190878) 2026-08-11 jmccandless@google.com Include the examples cross imports checker in the analzyer. (flutter/flutter#190674) 2026-08-11 bkonyi@google.com [devicelab] Remove orphaned screenshot test files (flutter/flutter#190879) 2026-08-11 jason-simmons@users.noreply.github.com Remove the bringup flag from the linux_arm_host_desktop_engine builder (flutter/flutter#190935) 2026-08-11 mdebbar@google.com Reduce web_skwasm_tests subshards from 8 to 2 (flutter/flutter#190728) ...
fixes #190395
This replaces
TextShadowCacheKey::identifier, which was a cast of the TextFrame pointer, with a content aware fingerprint. This allows cache hits across frames. It also removes the possible error where the pointer is recycled for a different TextFrame on the next frame.The fingerprint approach was chosen over storing a shared_ptr to a TextFrame and performing equality checks on it. The fingerprint avoids shared_ptr and indirection overhead in the TextShadowCacheKey. Instead of just relying on the hash we added extra fields to create the full fingerprint to make the probability of a false cache essentially zero (the texts on the same frame would have to have the same length, start, end, and hash code to create a false cache hit).
I measured this locally with a windows machine with the reproduction from #190395, after this the shadowed text frames measure the same as the non-shadowed text frames.
#190861 adds a benchmark, the PR was measured on macos with that benchmark as well.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.