Try rasterizing each picture and layer at most once. Apply a consistent caching treshold to layers and pictures.

[ignatz]

If Rasterization fails, i.e. image.is_valid() is false, the cache might try rasterizing the image again on the next frame. Not only is this wasteful put might also prevent other pictures to be cached within the current frame budget.

CC @cbracken

[cbracken]

[cbracken]

@ignatz do you have any preliminary numbers on the performance effect here? (We collect them on post-submit in the engine-framework roll either way)

[cbracken]

Hmm -- this is consistently (three runs in a row) failing the build_and_test_linux_unopt_debug bot. According to Cirrus, it's being killed by the OOM killer.

[ignatz]

@ignatz do you have any preliminary numbers on the performance effect here? (We collect them on post-submit in the engine-framework roll either way)

Adrian has exact numbers but to paraphrase him: this change helps measurably but the frame drops don't significantly improve since the dominating factor is still the high render cost on the gpu thread. This change helps only indirectly by reducing the opportunity cost of not rasterizing other elements on screen. The gains heavily depend on "rasterizations-per-frame" (peaking at one, i.e. nothing else gets rasterized while unrasterizable elements are on screen) and the relative savings for the other elements.

[ignatz]

Hmm -- this is consistently (three runs in a row) failing the build_and_test_linux_unopt_debug bot. According to Cirrus, it's being killed by the OOM killer.

I have a suspicion. In the test I create a large SkPicture to make it fail rasterization. Which works fine on my workstation given enough memory but might be to greedy for the more constraint CI environment. Maybe you know of another nifty way to make rasterization fail? Otherwise, I'm also happy to experiment with more conservative numbers tomorrow.

[ignatz]

Hmm -- this is consistently (three runs in a row) failing the build_and_test_linux_unopt_debug bot. According to Cirrus, it's being killed by the OOM killer.

I have a suspicion. In the test I create a large SkPicture to make it fail rasterization. Which works fine on my workstation given enough memory but might be to greedy for the more constraint CI environment. Maybe you know of another nifty way to make rasterization fail? Otherwise, I'm also happy to experiment with more conservative numbers tomorrow.

I did update the test. Hopefully it passes now within the memory limits. Generally the testing approach is kinda brittle, since the renderable bounds depend on the environment. That said, I don't have a better approach. A mockable skia api would be nice.

Seems to be still failing :/

Note, since your last review I piggy bagged a semantic change into this PR. Previously we would always cache layers and not respect any access thresholds. The fewest layer types actually use the cache. However, I've seen per-frame re-rasterizations of text during opacity animations. I added a test but can totally understand if you'd like me to pull the changes apart. Just let me know

[ignatz]

Hmm -- this is consistently (three runs in a row) failing the build_and_test_linux_unopt_debug bot. According to Cirrus, it's being killed by the OOM killer.

I have a suspicion. In the test I create a large SkPicture to make it fail rasterization. Which works fine on my workstation given enough memory but might be to greedy for the more constraint CI environment. Maybe you know of another nifty way to make rasterization fail? Otherwise, I'm also happy to experiment with more conservative numbers tomorrow.

I did update the test. Hopefully it passes now within the memory limits. Generally the testing approach is kinda brittle, since the renderable bounds depend on the environment. That said, I don't have a better approach. A mockable skia api would be nice.

Seems to be still failing :/

Turns out I optimized the wrong way. Concretely, I made the bounds smaller in the test rather than bigger. The problem seems to have been that while my workstation was failing with more conservative bounds the CI environment wans't, which lead the CI environment to actually allocate the memory and in turn exceeded the container limits.
Now making the bounds more progressive the test seems to pass. PTAL

[ignatz]

ping

[cbracken]

/cc @liyuqian for further review.

[liyuqian]

LGTM with one nit. I'd also love to see a companion flutter/flutter pull request that adds a performance test of this so we can quantify the improvement. Although this may not directly affect the Opal dropped frame metrics since there are other bottlenecks as #16545 (comment) suggested, I'm pretty sure that the frame_rasterization_time_millis in Flutter's performance test would capture some improvement of this PR.

If this PR is not urgent, it's also better to land that performance test PR first so our dashboard would record the numbers before and after this improvement. I'm very happy to provide help writing the performance test. My goal is let more and more people feel comfortable at writing Flutter performance tests.

[ignatz]

LGTM with one nit. I'd also love to see a companion flutter/flutter pull request that adds a performance test of this so we can quantify the improvement. Although this may not directly affect the Opal dropped frame metrics since there are other bottlenecks as #16545 (comment) suggested, I'm pretty sure that the frame_rasterization_time_millis in Flutter's performance test would capture some improvement of this PR.

If this PR is not urgent, it's also better to land that performance test PR first so our dashboard would record the numbers before and after this improvement.

This is certainly not the most high-impact improvement. Urgency is relative, would be good to have an ETA. It would still be a nice to have change, since not keep rasterizing non-rasterizable items seems like a strict improvement. The effective performance gain will depend on:

1. How expensive the failure is
2. Number of rasterizations per frame. I.e there's an opportunity cost, you might completely starve rasterization.

Note that I'd only expect a measurable improvement if the mentioned test induces failed rasterizations. Depending on that it might not even be worth waiting for it.

I'm very happy to provide help writing the performance test. My goal is let more and more people feel comfortable at writing Flutter performance tests.

Right, the main problem here is probably that it's non-trivial and brittle to set up failed rasterizations from flutter. I'm aware of only one way.

I think any such performance test would be super helpful going forward to avoid future regressions. For this specific change, it would just state the obvious: less work more better :).

Independently note that there are still some test infra failures because of the test that tries to achieve failed rasterizations by creating an absurdly large image. Unfortunately, this is brittle since it can depend on your rendering environment. What do you suggest, are you aware of a better way to fail rasterization, can we disable the test in certain environments (e.g. preprocessor defines), or any other ideas?

[liyuqian]

@ignatz by looking at our code, having a super large cull rect seems the only way other than subclassing RasterCache and override RasterCache::Prepare. BTW, I don't see how the test failure is related to your code or memory usage... They just failed to compile some unrelated code. Can you please try to rebase your PR with master engine and see if those tests would pass?

[ignatz]

BTW, I don't see how the test failure is related to your code or memory usage... They just failed to compile some unrelated code. Can you please try to rebase your PR with master engine and see if those tests would pass?

Happy to hear \o/. Rebased

[liyuqian]

@ignatz : the regression suggests that there's something that we missed in this PR since this should be a strict improvement. To figure out what we missed, run the performance test with a locally built engine:

• https://github.com/flutter/flutter/wiki/Compiling-the-engine
• run animated_placeholder_perf driver test by cd into the directory of https://github.com/flutter/flutter/tree/master/dev/benchmarks/macrobenchmarks, and run flutter drive --profile --local-engine=android_profile animated_placeholder_perf
• Check the 90th percentile rasterization time

[liyuqian]

@ignatz : thanks to @flar , we now believed that regression flutter/flutter#51776 of this PR is caused by changing layer raster cache's behaviors. Before this PR, we'll always raster cache a layer if possible. This PR unexpectedly introduces the picture raster cache access threshold to the layer raster cache in https://github.com/flutter/engine/pull/16545/files#diff-d94c1943874763e8d6cc9eed9f1ab7eaR130. I unfortunately missed that line (and the title of this PR) during the code review, as I thought this PR only fixes how we increase the layer raster cache access count without actually enforcing the threshold.

@ignatz : would you like to reland this patch without forcing the threshold on the layer raster cache? You might want to change ThresholdIsRespectedForLayers unit test to ThresholdIsIgnoredForLayers .