Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Retained Rendering #21756

Closed
liyuqian opened this issue Sep 12, 2018 · 17 comments
Closed

Enable Retained Rendering #21756

liyuqian opened this issue Sep 12, 2018 · 17 comments
Assignees
Labels
c: new feature Nothing broken; request for a new capability c: performance Relates to speed or footprint issues (see "perf:" labels)
Milestone

Comments

@liyuqian
Copy link
Contributor

liyuqian commented Sep 12, 2018

This is for tracking all related pull requests for enabling retained rendering.

In my local experiments, the retained rendering can speedup fading transition by 2.15x and put our flutter_gallery__transition_perf average_frame_rasterizer_time_millis well below 16ms on Moto G4. Hence it will fix #13736

@liyuqian liyuqian added c: new feature Nothing broken; request for a new capability c: performance Relates to speed or footprint issues (see "perf:" labels) labels Sep 12, 2018
@liyuqian liyuqian self-assigned this Sep 12, 2018
@liyuqian
Copy link
Contributor Author

liyuqian commented Sep 12, 2018

CC @Hixie @cbracken @chinmaygarde @jason-simmons @yjbanov @amirh

Let's try to close this issue before the end of October so we can squeeze another 30% speedup?

We'll also have to check the memory usage (thanks Chinmay for the remind). My guess is that the memory cost should be much less compared to our current raster cache: in my experiments, I only add the raster cache when there's an animated opacity. Those things should be much rarer than RepaintBoundaries and PictureLayers. Chinmay and I also think that we shall provide the raster cache option to the developers just like what we did with clipping.

@yjbanov
Copy link
Contributor

yjbanov commented Sep 13, 2018

we shall provide the raster cache option to the developers just like what we did with clipping

What would the API look like?

@liyuqian
Copy link
Contributor Author

For example, the constructor of AnimatedOpacity could take one more parameter cacheBehavior which has 3 possible values Cache.always, Cache.never, Cache.auto.

@liyuqian
Copy link
Contributor Author

Just found a bug in my experimentation which caused my program to never reuse the cache...

So the 30% speedup is just by replacing saveLayer with taking snapshot and then paint. So this part is memory-free.

After fixing the bug, the speedup is actually ~2.125x

@Hixie
Copy link
Contributor

Hixie commented Sep 17, 2018

Can you elaborate on what "retained rendering" entails?

@liyuqian
Copy link
Contributor Author

Yes, retained rendering in my current experimental implementation means

  1. On the framework side, maintain which Layer is dirty, and which Layer subtree has dirty Layers.
  2. On the engine side, make std::shared_ptr<flow::Layer> instead of std::unique_ptr<flow:Layer> and send such shared layer pointer to the Dart side during the SceneBuilder::pushXXX call.
  3. During Layer::addToScene phase, check if the subtree is dirty or not. If so, proceed as before. If not, reuse the shared layer pointer (retained layer subtree) and immediately stop the subtree walk from there.
  4. During the Paint/Preroll of some special engine layer (e.g., OpacityLayer), take a raster snapshot of the whole subtree, and reuse it if the whole subtree is retained and the matrix is unchanged other than translation.
  5. For flow::PhysicalShapeLayer in Fuchsia, we can skip the Paint entirely if the subtree is retained and the matrix is unchanged other than translation.

@yjbanov
Copy link
Contributor

yjbanov commented Sep 19, 2018

Will changing AnimatedOpacity alone be effective or will we also require that the child of AnimatedOpacity be wrapped in a RepaintBoundary?

Also, we should probably be consistent with the methods we provide to developers for controlling the raster cache. For example, we already have a concept of willChange. Perhaps we should use the same flag in AnimatedOpacity. Alternatively, we could change the existing flag to use the Cache enum before we freeze the API. I'm leaning towards the enum, because it is extensible in the future. A bool flag will be hard to extend.

@liyuqian
Copy link
Contributor Author

Child of AnimatedOpacity definitely won't have to be wrapped in a RepaintBoundary because we already automatically consider the Opacity as a RepaintBoundary.

I also prefer willChange to take an enum, and reuse it in retained rendering.

@yjbanov
Copy link
Contributor

yjbanov commented Sep 24, 2018

we already automatically consider the Opacity as a RepaintBoundary.

Do you mean the engine treats it as a repaint boundary or the framework? I'm not aware of anything in the framework that treats it as a repaint boundary, and so the framework will hand you a new Picture on every frame. Perhaps I'm not looking in the right place?

@liyuqian
Copy link
Contributor Author

The framework has alwaysNeedsCompositing = true if Opacity has child and it's not fully opaque or transparent: https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/rendering/proxy_box.dart#L732

@yjbanov
Copy link
Contributor

yjbanov commented Sep 24, 2018

AFAICT, alwaysNeedsCompositing does not affect what the framework repaints. It only causes an opacity layer created for the Opacity widget. When the opacity changes the opacity setter is called, which in turns requests a repaint. This will repaint the entire subtree under the Opacity widget and generate a brand new Picture, unless there's a RepaintBoundary to stop it.

BTW, RepaintBoundary may not be sufficient. You need something that will prevent re-layouts from being propagated from the child to the Opacity widget. IOW, something that causes RenderObject._relayoutBoundary to be true, such as a SizedBox or something.

@liyuqian
Copy link
Contributor Author

Oh, you're right. In my experiments, the RepaintBoundary is not provided by Opacity or AnimatedOpacity, but by the ModalRoute. It works for me because my AnimatedOpacity is always applied to a page transition between two ModalRoute.

Considering the cost of Opacity and AnimatedOpacity, I wonder if we should set isRepaintBoundary => true there just like other heavy RenderObject such as TextureBox and RenderView.

In any case, I did plan to add contraints to engine OpacityLayer so it should only have one child instead of multiple children. I thought that this was always satisfied because of the RepaintBoundary and its single child OffsetLayer. If this is not the case, then I'll have to make it so by introducing RepaintBoundary.

So to answer your original question, yes we'll have Opacity and AnimatedOpacity to be RepaintBoundary so their children is wrapped into a single OffsetLayer.

@Hixie
Copy link
Contributor

Hixie commented Sep 25, 2018

I'm not sure I understand how the dart:ui API is expected to look after this change. Can you elaborate on this?

@liyuqian
Copy link
Contributor Author

For CustomPaint widget, we'll have

class CustomPaint extends SingleChildRenderObjectWidget (
  ...
  this.cacheBehavior = Cache.auto
)

instead of

class CustomPaint extends SingleChildRenderObjectWidget (
  ...
  this.isComplex = false,
  this.willChange = false
)

as isComplex and willChange only affect raster cache behavior so it's probably more clear to just let developers decide whether to cache it or not.

Similarly, for Opacity/AnimatedOpacity, we could have

class Opacity extends SingleChildRenderObjectWidget (
  ...
  this.cacheBehavior = Cache.always (or, Cache.auto, Cache.never)
)

@liyuqian
Copy link
Contributor Author

As discussed with @Hixie , we probably won't change the APIs of CustomPaint, and we'll probably add cacheBehavior to RepaintBoundary.

liyuqian added a commit that referenced this issue Sep 28, 2018
For retained rendering, we don't want to push the offset down to each leaf layer. Otherwise, changing an offset layer on the very high level could cascade the change to too many leaves, which means that we can't retain them.

To not push the offset downwards, we simply push a TransformLayer when there's an offset. Skia has a fast path for concatenating scale/translation-only matrix so this operation should be fast (no performance regression is measured on Moto G4).

This is our first step towards #21756
liyuqian added a commit to flutter/engine that referenced this issue Oct 2, 2018
To make the PR minimal, we currently only share the engine layer when `pushPhysicalShape` (for Fuchsia) or `pushOffset` (for `RepaintBoundary` and `Opacity`) are called. They should be sufficient for our short-term perf goal. In the future, we can incrementally share more engine layers with the framework.

flutter/flutter#21756
liyuqian added a commit to flutter/engine that referenced this issue Oct 11, 2018
We first test this with OpacityLayer. This test alone (without retained rendering) should have ~30% speedup as we'll have fewer render target switches by snapshoting in the Preroll instead of saveLayer in the Paint.

In my local flutter_gallery transition perf tests, the average frame time drops from ~16ms to ~12ms.

flutter/flutter#21756
@liyuqian
Copy link
Contributor Author

#23434 completes this feature request. We'll track more exploratory work in retained rendering in a new issue: #23535

@github-actions
Copy link

github-actions bot commented Sep 1, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: new feature Nothing broken; request for a new capability c: performance Relates to speed or footprint issues (see "perf:" labels)
Projects
None yet
Development

No branches or pull requests

3 participants