The referenced issue flutter/flutter#101872 doesn't seem to have any test cases to demonstrate the problem it assumes. It does point to problems in flutter/flutter#101597 though which are due to a more fundamental problem in how Flutter manages animations. They need to fix that and so many other cases will improve as well.

This solution, while it might help a little with some test cases (which? it doesn't cite any), only targets a particular (and alleged) symptom of the problem whereas solution is available to the framework and app developers that will obviate the need for this PR and others like it.

Let's fix these issues from the top down.

See examples presented in these comments:

• Opacity/FadeTransition should allow child widgets to avoid repainting flutter#101597 (comment)
• Opacity/FadeTransition should allow child widgets to avoid repainting flutter#101597 (comment)

@flar
I guess about the two test cases of the issue flutter/flutter#101597, @jonahwilliams hopes that developers can solve it without manually adding RepaintBoundary.

case 1 flutter/flutter#101597 (comment)

case 2 flutter/flutter#101597 (comment)

From case2, if we decide not to add RepaintBoundary manually, and the PR of the framework will be landed in the future, then this PR makes sense.

cc @jonahwilliams

The point I'm making is that there are other reasons for them to add a RepaintBoundary and we should work on dealing with that (perhaps having the standard AnimatedFoo classes all do it for them or something). Making this change in the engine for that reason means we are providing fewer reasons for them to make the right change.

Also, this change addresses one stability problem. If the children are not a picture or dl layer, but they are stable, we don't get any benefit from this change. The animating widgets need to ensure the stability of their children whether or not it is a simple picture layer and since they don't do that, this PR has only solved the problem for animators with picture children.

The RepaintBoundary isn't there to provide us a caching opportunity. That's not why it was created. It was created to reduce work in the framework so it doesn't have to visit as many nodes in the tree to rebuild on every frame. That saves a bunch of work in the "Build" phase of the tree that we don't even see.

At some point we added the picture cache which took advantage of the isolation and stability that the RepaintBoundary provides. We'll still get that benefit, and we'll get it more effectively if the framework were to work on its sub-tree stability better.

I do like the idea that caching a DisplayListLayer should be equivalent to caching its embedded DisplayList directly.

Is there a simpler solution with, say, a layer override. Some way that we say "layer cache yourself" and it says "you mean, cache my DL"? We wouldn't have to mess with rearranging the cache IDs, just implement "when you try to cache a DL layer, it will catch that and cache its DL instead".

The only tricky part is if a parent layer says "cache my children" and the child is singular, but do we already catch that?

@ColdPaleLight flutter/flutter#101952 has been updated to keep the child picture stable for the AnimatedSwitcher

The point I'm making is that there are other reasons for them to add a RepaintBoundary and we should work on dealing with that (perhaps having the standard AnimatedFoo classes all do it for them or something). Making this change in the engine for that reason means we are providing fewer reasons for them to make the right change.

I don't disagree with you here, but I think that as long as we are going to attempt to cache children we should do it correctly. Or bail out and avoid populating a cache we'll never hit

@jonahwilliams @flar
I filed another PR here #32738 to implement Option 1 mentioned in the Issue. Perhaps that is more suitable for solving linked issue.

This change seems fairly straightforward compared to a breaking API change in dart:ui.

A recent version of the "cacheable list" changes the way we manage layers in the cache in a way that might help us consolidate "DLLayer caching == DL caching". I feel like the design is still a little convoluted, but this mechanism/concept might give us back some control over what is being cached and how.

See #31892

A recent version of the "cacheable list" changes the way we manage layers in the cache in a way that might help us consolidate "DLLayer caching == DL caching". I feel like the design is still a little convoluted, but this mechanism/concept might give us back some control over what is being cached and how.

See #31892

Hi @flar
Sorry, but I'm not sure if I didn't understand what you meant due to the language barrier, or if you misunderstood the intent of this PR.

This PR wants to solve the problem that kChildrenLayer will fail If DisplayListLayer is unstable but DisplayList is stable, here is linked issue for details flutter/flutter#101872 .

I thought maybe "DLLayer caching == DL caching" would be nice, but that mechanism doesn't seem to do anything for the issue we would like to solve. Please let me know if I missed something.
Thank you very much.

There are a number of outstanding issues right now to change our raster cache and they are starting to blend together in my head. The top concern for me is that if we have to do this much work on the raster caching code, and if each solution involves new mechanisms on top of old mechanisms, then maybe we are too deep in the grass and need to stand up and re-assess.

In particular, in your table above, flutter/flutter#101952 is deemed as not solving the problem, but I think it is the right solution here. It seems to be an ongoing work and it might just solve the problem on its own if we wait for it to settle.

I strongly support that any animation:

• isolate itself from disturbing the tree above it. A RepaintBoundary above it can do that, but the "AnimatingFoo" paradigm side steps that by have a builder phase in its RO that rebuilds without disturbing the tree. That's a win.
• not rebuild its children if they don't need to rebuild. [framework] allow other RenderObjects to behave like repaint boundaries flutter#101952 seems to be trying to address that, but I'm not sure it is quite there yet (I need to look at it more closely, but I'm not a framework expert).

We have tools to address those principles above and we should make use of them as much as possible before complicating the raster cache much further.

Having said that, this PR is not a huge complication, but it does add a few more steps and a couple more classes. We should wait and see what we can accomplish with flutter/flutter#101952 as this PR alone doesn't solve the problem itself per the above table. If we can't get there with 101952 alone, then this PR seems like a good way to help with that.

@jonahwilliams @flar
flutter/flutter#101952 has landed, and I tested it locally and found that the two test cases of the issue flutter/flutter#101597 still not work yet. So we still need to land this PR, right?

Overall this looks pretty clean. I've made a couple of suggestions that might improve performance. Do we have any benchmarks that measure the cost of caching (prepare/draw/etc) with these more complicated keys?

I tested it with the following code:

1. With this PR

  auto start = fml::TimePoint::Now();
  RasterCacheKey::Map<int> map;
  auto hash_function = map.hash_function();
  SkMatrix matrix = SkMatrix::I();
  for (int i = 0; i < 100000; i++) {
    RasterCacheKeyID first = RasterCacheKeyID(foo, RasterCacheKeyType::kLayer);
    RasterCacheKeyID second = RasterCacheKeyID(bar, RasterCacheKeyType::kLayer);
    RasterCacheKeyID third =
      RasterCacheKeyID({first, second}, RasterCacheKeyType::kLayerChildren);
    RasterCacheKey key(third, matrix);
    hash_function(key);
  }
  auto end = fml::TimePoint::Now();
  FML_DLOG(INFO) << "cost " << (end - start).ToNanoseconds();

2. Without this PR：

  auto start = fml::TimePoint::Now();
  RasterCacheKey::Map<int> map;
  auto hash_function = map.hash_function();
  SkMatrix matrix = SkMatrix::I();
  for (int i = 0; i < 100000; i++) {
    RasterCacheKey key(RasterCacheKeyID({foo, bar}), RasterCacheKeyType::kLayerChildren, matrix);
    hash_function(key);
  }
  auto end = fml::TimePoint::Now();
  FML_DLOG(INFO) << "cost " << (end - start).ToNanoseconds();

With this PR：
total 315889084 ns， Average time per key is 315 ns.

Without this PR:
total 136134995 ns, Average time per key is 136 ns.

So I think this PR will not bring significant performance issues.

From @flar s last comment, there was some skepticism about landing this. Is that correct?

I feel like with the latest design for #31892 this will be largely unnecessary or perhaps require a slightly different implementation.

I feel like with the latest design for #31892 this will be largely unnecessary or perhaps require a slightly different implementation.

@flar
I think this PR is still needed. In this PR, it changed the generation rules of child layer ids, which is not implemented in PR #31892 (of course, it does not need to care about this).

I made a slight adjustment to this PR, removed the layer::caching_key_id method and instead provided the RasterCacheKeyID:::LayerChildrenIds method (same as in the PR #31892). I also discussed with @JsouLiang offline, he will resolve the conflict after this PR is landed.

There is already a lot of pressure on #31892. I'm glad that @JsouLiang is willing to accommodate this, but I'm not willing to go through the review churn on #31892 to deal with any more distractions. Right now that PR is my main focus and I'm not going to be reviewing any more churn on the RasterCache mechanism until we get that vital PR landed.

Just glancing briefly through this solution, it looks like it will recache a DL if we add a filtering layer on top of it because the cache of the DL layer will move from a DL cache entry to a children cache entry with the DL as its only child. We should be reusing the same DL cache entry instead of changing its type.

Moving beyond #31892 we have flutter/flutter#104303.

We need to start thinking outside of our current "implementation box" to make some real progress across the board. Every time we add just one more new mechanism on top of the old to fix a single condition here or there we are making the situation more entwined and harder to innovate within.

At least #31892 starts us towards a wiser mechanism by consolidating all of the caching requests into a single queue, but its first implementation will be limited to just providing a reorganization of the current narrowly focused logic. We need to look at ways to have more sophisticated multi-layer logic once that consolidation has brought the information into a single queue for us to analyze.

@flar , I know the raster cache is being refactored, so I didn't intend to update this PR originally (since you commented yesterday, I explained and updated it a bit). Let's wait for the PR #31892 land and then come back to this proposal.

Just glancing briefly through this solution, it looks like it will recache a DL if we add a filtering layer on top of it because the cache of the DL layer will move from a DL cache entry to a children cache entry with the DL as its only child. We should be reusing the same DL cache entry instead of changing its type.

I guess there must be some misunderstanding here. I believe this proposal has no impact on the mechanics of the raster cache except that the rules for generating the RasterCacheKey have changed a little bit.

Let's wait for the PR #31892 land and then come back to this proposal.

Can we close this PR in the meantime?

As long as we don't forget about it. I'll leave it to @ColdPaleLight to close it or move it to draft status.

Reopen it since #31892 has been landed.

@flar Since #31892 has been landed, would you mind taking a look at this PR again when you have a chance? Thank you :)

Should we have a benchmark that looks at how much time we spend creating IDs, cache items, cache entries, etc. so that as we make these processes more complicated we don't start adding visible overhead?

Should we have a benchmark that looks at how much time we spend creating IDs, cache items, cache entries, etc. so that as we make these processes more complicated we don't start adding visible overhead?

Good idea, filed a issue for it flutter/flutter#107811