[WIP]RasterCache adds a new mechanism for particularly complex pictures or display lists #31925
Conversation
ColdPaleLight
force-pushed
the
particularly_complex_cache_entry
branch
from
d2aea6a
to
cadaf84
Compare
|
In principle, I like this change, for the issues described in the bug. The only potential downside is that we cache more than we were caching before - but I think it is rare that the framework would display one 1 frame for something. most of the short animations are ~100ms at least. |
|
I think the bigger question is: if this change is worthwhile, then what is the downside of always caching on the first frame? |
There was a problem hiding this comment.
We already have a flag for complexity and it is called "is_complex". We should not make a new flag that is "something something AND complex". We should keep the flags orthogonal. If something isn't complex then it is slower to cache it than it is to render it from scratch so providing yet another way for developers to defeat the complexity scoring is just another rope for them to hang themselves with.
I was hesitant before whether to add a new flag. If |
ColdPaleLight
force-pushed
the
particularly_complex_cache_entry
branch
from
a00ae5e
to
0a4e7c0
Compare
|
Hi, @flar |
Oh dear, my comment was taken the wrong way. My comment was about the meaning of the new flag, not whether or not to add one. We have is_complex to mean the drawing is complex. You were adding "is_high_priority" and making it mean "is_complex" as well. I'm OK with 3 flags, but their meaning should be orthogonal. "is_high_priority" should only affect "when" we cache, not "whether it is complex enough to cache". In particular, you had this code (in both the picture and display_list code): The "high priority" flag should not be overriding our metrics as to whether or not the picture is worth rasterizing. If something is not worth rasterizing then we should not rasterize it even if it is high priority. They already have the ability to set "is_complex" to override that logic. We should not have another flag also indicate complexity. For whatever changes "high_priority" makes to our caching logic, the parts that it should not impact are the parts that are already controlled by "is_complex". Instead "high_priority" should impact whether we count down from 3 before we cache it and whether we keep it after its last use for a few frames, but it should not impact our complexity rating. |
|
In short, I think what was intended here was 3 flags, meaning:
As your original PR was implemented, you had "is_high_priority" also interfering with our complexity evaluation - which is the job of "is_complex" and should not be a side effect of "is_high_priority". |
|
@flar Sorry, I misunderstood your comment. Thank you for your kind comments, I tweaked the code as you suggested, please take a look again, thanks! |
|
We recently revisited one of the issues that suggested this PR and were concerned that the needs were not adequately predetermined. In other words, this concept is a working hypothesis, not an immediate need. I'm tagging @dnfield and @jonahwilliams to weigh in on the empirical evidence that this particular solution might address, or how to collect such evidence and evaluate that this solution makes a difference. |
There was a problem hiding this comment.
Some minor nits and a potential logic change once we look at some more empirical data, but I think this concept is implemented well here.
I'm flagging this as "Request changes" only on the basis that I'd like to see more buy-in on the concept and a way to measure this empirically before we move forward.
| @@ -245,7 +247,8 @@ bool RasterCache::Prepare(PrerollContext* context, | |||
|
|
|||
| // Creates an entry, if not present prior. | |||
| Entry& entry = cache_[cache_key]; | |||
| if (entry.access_count < access_threshold_) { | |||
| entry.is_high_priority = is_high_priority; | |||
There was a problem hiding this comment.
Should we have entry.is_high_priority be a one-way latch, or a most-recently-specified flag? This might depend on what the recent request for empirical evidence turns up.
flow/raster_cache.h
Outdated
| std::unique_ptr<RasterCacheResult> image; | ||
| // Return the number of frames the entry survives if it is not used. If the | ||
| // number is 0, then it will be evicted when not in use. | ||
| size_t unused_threshold() const { return is_high_priority ? 3 : 0; } |
There was a problem hiding this comment.
Maybe move "3" to a class constant (something like kHighPriorityEvictionThreshold) so its meaning is more obvious and the number is more visible for when we want to tweak it.
flow/raster_cache.cc
Outdated
| if (entry.unused_count < entry.unused_threshold()) { | ||
| entry.unused_count++; | ||
| if (entry.image) { | ||
| RasterCacheKeyKind kind = it->first.kind(); |
There was a problem hiding this comment.
Maybe have a function metrics_for(kind) or metrics_for(<type of it.first>) to simplify all 3 cases where we have to choose a metrics to modify?
There was a problem hiding this comment.
done, added a new method GetMetricsForKind.
|
This has been tricky to nail down. I'm less convinced now that using the raster cache more will help us. Jonah may have more data on this, but in various experiments I tried using the raster cache more actually slowed things down. It'd help to see the benchmark(s) this helps. |
|
I used https://github.com/dnfield/flutter_svg/tree/master/example as benchmark and made a little tweak in final Timeline timeline = await driver.traceAction(() async {
await driver.scroll(
view,
0,
- -3400,
+ -1800
- const Duration(seconds: 10),
+ const Duration(seconds: 3),
timeout: const Duration(seconds: 15),
);
});It can be seen from the benchmark that if
Device: Xiaomi Mi 11 (M2102K1C)
with is_high_prioritysummary 1 summary 2 without is_high_prioritysummary 3 summary 4 But not in all scenarios, |
There was a problem hiding this comment.
I think this is an interesting idea, but we need to have a better motivating case for this. Even in the linked case, we can see that some SVGs benefit from this and others regress. Our customers are likely to not use this correctly.
We already have two flags that feed into raster caching behavior, and that customers don't really know how to use and/or rarely use.
What really need here are some solid benchmarks and docs that explain what we want to do with the raster cache. I personally have had a hard time coming up with motivating cases to show that using it more would be better, or that holding on to entries longer would be better. The GPU seems to start to get into a poor state when it has to work with too many textures, and making a change that will both be correct in most cases and be relatively easy for developers to use is hard.
|
Happy to discuss this more on discord and/or in a doc. |
|
What @dnfield said. I have been reviewing this PR mostly for "correctness in how it integrates with our caching mechanisms" and not "whether developers could or would take advantage of this flag as proposed". Dan knows much more about that end of things and so I encourage you to engage on the "why" of this PR with him. |
|
@dnfield You're right, we need more information to help us decide whether this feature will work as well as expected. I think I'll have to do some more in-depth and comprehensive research I believe Test: color_filter_and_fade_perf_test run with run without |
ColdPaleLight
changed the title
|
I investigated the two features that
|
Doesn't flutter have a mechanism by which you can express a range around the visible field during which the elements will be included in the tree? If so, then they might remain cached if they are just off the screen. I think when I found it one day I asked around and there may be some issues with how it is implemented. If we can get that mechanism to work right, then the above conditions may fall out from it. We'd also have to look into how we "RasterCache::Touch" the entries when they are in that "included in the tree, but not visible" state. |
|
Other things to consider - does the extra memory for "potentially reusable" cache entries negatively impact apps? Does it cost extra battery to keep the app warm or to wake it up on a phone with a lot of background tasks? (Note - fixed a typo. If it actually does cost extra "batter" then we must address this extreme waste of uncooked baked goods...) |
This is common in ListViews, if an item scrolls out of the visible area but is still in the cache area, it will be cached instead of disposed. c.f. |
RasterCach::Prepareadds the new parameteris_high_priorityto specify whether the raster cache entry is high priority. If the raster cache entry is high priority, it will always cache on first usage and survive 3 frames without usage.Partially fix issues:
flutter/flutter#87827
flutter/flutter#87826
If this PR can land, the following PRs will expose this mechanism to
Layerofdart:uiand add zombie algorithm as mentioned in flutter/flutter#87827 (comment)cc @flar @jonahwilliams
Pre-launch Checklist
writing and running engine tests.
///).