[Impeller] Improve performance of CupertinoPicker with opacity peephole #40101

jonahwilliams · 2023-03-07T00:19:09Z

Implements an EntityPassDelegate that applies an opacity peephole optimization. This has been tuned to be extremely conservative, and does not successfully remove all opacity layers from the cupertino example linked. Skia on the other hand does manage to, so perhaps it is using more precise coverage information from the glyphs.

Impeller Before

Impeller After

Skia

flutter/flutter#121476

jonahwilliams · 2023-03-07T00:31:10Z

TBD: whether we should do it like this, testing, et cetera

jonahwilliams · 2023-03-07T00:35:54Z

impeller/aiks/paint_pass_delegate.cc

+  // command wrapped in save layer. This would indicate something like an
+  // Opacity or FadeTransition wrapping a very simple widget, like in the
+  // CupertinoPicker.
+  if (entity_pass->GetEntityCount() > 3) {


One concern I have with working on this optimization now: subpasses are extra expensive right now on the metal backend due to the whole waitForScheduled, and also potentially due to lack of multithreaded encoding.

We might tune these thresholds too high and end up trading a bunch of raster time for decreasing returns of GPU utilization.

Yes! We're terribly underutilizing the GPU here. Sibling EntityPasses are fundamentally independent and can be evaluated in parallel on the CPU (not a huge impact in this case) and executed in parallel on the GPU (absolutely enormous impact in this case). Once we have a proper synchronization solution, we can remove all of the host<->device syncs that are blocking the CPU and saturate the GPU with parallelizable work.

Although I think we added texture atlas sharing between EntityPasses at some point. That (and any other resource sharing between EntityPasses) needs to be reverted, of course.

Can we get around that by making glyph atlas construction happen eagerly? Otherwise, glyph atlas construction and upload takes a substantial amount of time.

or, more specifically, can we push construction of the glyph atlas including all text frames into a different thread and then use whatever resource tracking mechanism we create to ensure the subsequent commands that depend on this are ordered correctly?

Can we get around that by making glyph atlas construction happen eagerly

Yeah I think that's probably the best option -- just render the whole thing at once and upload everything just before the EntityPass run. Then the uploaded atlases become read only for the frame and don't restrict parallelism.

jonahwilliams · 2023-03-07T17:50:52Z

impeller/entity/entity_pass.h

@@ -52,6 +52,13 @@ class EntityPass {

  void IterateAllEntities(const std::function<bool(Entity&)>& iterator);

+  /// @brief Iterates all entities on this pass, returning if there is a
+  /// subpass.
+  bool IterateAllFlatEntities(const std::function<bool(Entity&)>& iterator);


This is sort of an unnecessary restriction, but I want to avoid traversing the entire tree. Ideally if all children accepted opacity and there was a subpass, we could check if the subpass accepts opacity and just push it one layer down.

This would help cases where we've nested FadeTransitions/Opacity

chinmaygarde · 2023-03-07T23:26:39Z

impeller/entity/contents/contents.h

+  ///        a way that makes accepting opacity impossible. It is always safe
+  ///        to return false, especially if computing overlap would be
+  ///        computationally expensive.
+  virtual bool CanAcceptOpacity(const Entity& entity) const;


Can this just be a single call. Say "virtual bool InheritOpacity(...)". The return value can indicate if the operation succeeded.

We have to make sure the entities in a pass aren't overlapping too, so we can't combine them. basically there would need to be two levels of checks:

For each entity in the pass, it needs to be able to accept opacity and not overlap other entities.

Within an entities contents, its geometry needs to be non-overlapping.

jonahwilliams · 2023-03-09T16:49:51Z

From investigation, its not that Skia has better coverage info, its that the display list heuristics don't consider overlap: flutter/flutter#122266

jonahwilliams · 2023-03-10T18:12:34Z

I think this is ready for review. It works for the cupertino picker case, and we can continue to invest in it should we find additional cases where we create way too many subpasses

jonahwilliams · 2023-03-13T20:44:08Z

well, rebasing from github is hard!

dnfield · 2023-03-13T22:11:21Z

impeller/aiks/canvas.cc

-  new_layer_pass.SetDelegate(
-      std::make_unique<PaintPassDelegate>(paint, bounds));
+
+  // Only apply opacity peephole on default blending.


Does this deserve a TODO/bug to extend this to other blend modes?

I don't think we can in general.

dnfield · 2023-03-13T22:13:41Z

impeller/aiks/paint_pass_delegate.cc

+  }
+  bool all_can_accept = true;
+  std::vector<Rect> all_coverages;
+  auto had_subpass = entity_pass->IterateAllFlatEntities([&](Entity& entity) {


nit: avoid over-capturing (don't use [&]). Instead explicitly capture what you need.

dnfield · 2023-03-13T22:15:40Z

impeller/aiks/paint_pass_delegate.cc

+    auto maybe_coverage = contents->GetCoverage(entity);
+    if (maybe_coverage.has_value()) {
+      auto coverage = maybe_coverage.value();
+      for (const auto& cv : all_coverages) {


Can we make this even simpler by just bailing out if we don't get the structure in the comments above (i.e. clip, draw, restore)?

A small change above could lead to a very expensive change down here without realizing it.

We don't have a good way to check what kind of entity something is.

dnfield · 2023-03-13T22:18:04Z

impeller/entity/contents/text_contents.cc

@@ -4,6 +4,7 @@

 #include "impeller/entity/contents/text_contents.h"

+#include <iostream>


Suggested change

#include <iostream>

Not sure if github is showing that right, but what I'm trying to articulate is that this line can be deleted.

dnfield · 2023-03-13T22:23:14Z

impeller/typographer/text_frame.cc

+    return true;
+  }
+  auto glyph_positions = runs_[0].GetGlyphPositions();
+  if (glyph_positions.size() > 10) {


Why this?

What happens if there are 100 TextFrame objects each with 10 glyphs? This seems like the wrong level for optimization.

If there is more than 1 run or more than 10 glyphs, we assume that its not worth computing the overlap - instead just assume the text frame has overlapping contents and don't apply the optimization. Seems reasonable to me?

But why would we bail on 1 run that's 11 glyphs long, but chug along merrily on 1000 runs that are 10 glyphs long?

L51, we don't

So we can't get multiple TextFrames in a single pass?

Sorry, I'm not sure what you mean?

discussed offline, we need to do these checks because text rendering behaves differently than solid color fills. The optimization has a limitation in the number of entities we check per frame which is now documented on the PaintPassDelegate

jonahwilliams · 2023-03-15T01:08:43Z

This also helps two places in the gallery:

Crane: Each image fades in individually. With this optimization that takes no sub passes
Rally: several of the icons have opacity applied when not focused. Small subpasses, but still good to get.

bdero

LGTM!

bdero · 2023-03-15T01:41:42Z

impeller/entity/entity_pass.h

@@ -52,6 +52,13 @@ class EntityPass {

  void IterateAllEntities(const std::function<bool(Entity&)>& iterator);

+  /// @brief Iterates all entities on this pass, returning if there is a
+  /// subpass.


Nit: "Iterate entities in this pass up until the first subpass is found. This is useful for limiting look-ahead optimizations."

bdero · 2023-03-15T01:41:44Z

impeller/entity/entity_pass.h

@@ -52,6 +52,13 @@ class EntityPass {

  void IterateAllEntities(const std::function<bool(Entity&)>& iterator);

+  /// @brief Iterates all entities on this pass, returning if there is a
+  /// subpass.
+  bool IterateAllFlatEntities(const std::function<bool(Entity&)>& iterator);


Small nit, but maybe rename to "IterateUntilSubpass"?

…ity peephole (flutter/engine#40101)

dnfield · 2023-03-15T18:25:17Z

(My concerns have been addressed)

…ity peephole (flutter/engine#40101)

…122746) * 3c3cbc738 Manual roll Dart SDK from c766fffb626e to 3b109a04f783 (9 revisions) (flutter/engine#40296) * dee0ae590 [Impeller] Make matrix image filters work as expected with nested saving layers (flutter/engine#40299) * 835759f4c Roll Skia from 4d90ba479527 to 3b9131c65c01 (5 revisions) (flutter/engine#40304) * bbde3a77b Roll Fuchsia Linux SDK from BRE9jdqYpdkbU0j7H... to YaWqKKuj-fAqfpKCm... (flutter/engine#40306) * 1a7e7c468 Roll Dart SDK from 3b109a04f783 to 5c210933cdfe (2 revisions) (flutter/engine#40307) * 66a3324cf Analyze more shaders (flutter/engine#40285) * 405c8513e [Impeller] Improve performance of CupertinoPicker with opacity peephole (flutter/engine#40101) * 9fc3246bf Reland "Make FlutterTest the default test font" (#40188) (flutter/engine#40245) * 3ed9f1236 Reland: "Added wide-gamut color support for `ui.Image.toByteData` and `ui.Image.colorSpace`" (flutter/engine#40312) * e143b309c Bump lower Dart SDK constraints to 3.0 (flutter/engine#40178) * 086b1a022 [impeller] implement GetPositionUVBuffer (flutter/engine#40248) * ec151bf2c Revert Dart SDK to c766fffb626e (flutter/engine#40315)

jonahwilliams added 3 commits March 6, 2023 13:43

[Impeller] basic opacity peephole

7f9deb8

Merge branch 'master' of github.com:flutter/engine into little_peep

d2cb5ab

[Impeller] minimal opacity peephole for cupertino picker

e9c957c

jonahwilliams commented Mar 7, 2023

View reviewed changes

chinmaygarde requested review from bdero and chinmaygarde March 7, 2023 06:32

chinmaygarde assigned jonahwilliams Mar 7, 2023

chinmaygarde added the e: impeller label Mar 7, 2023

jonahwilliams commented Mar 7, 2023

View reviewed changes

add trivial unittest and update texture contents to accept opacity

b72fd09

chinmaygarde reviewed Mar 7, 2023

View reviewed changes

add more tests

454c7b3

jonahwilliams marked this pull request as ready for review March 10, 2023 18:05

jonahwilliams requested a review from chinmaygarde March 10, 2023 18:12

jonahwilliams added 2 commits March 10, 2023 10:28

++

966bf06

Merge branch 'main' into little_peep

d1145f2

fix compile error

1587dd6

jonahwilliams requested a review from dnfield March 13, 2023 22:07

dnfield reviewed Mar 13, 2023

View reviewed changes

jonahwilliams added 4 commits March 13, 2023 15:42

dont capture everything

fe306d7

remove iostream

080dc62

Merge branch 'master' of github.com:flutter/engine into little_peep

75c97b0

dont worry about geom overlap

1494463

jonahwilliams requested a review from dnfield March 15, 2023 00:23

bdero approved these changes Mar 15, 2023

View reviewed changes

bdero review

6dd0223

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 15, 2023

auto-submit bot merged commit 405c851 into flutter:main Mar 15, 2023
36 checks passed

engine-flutter-autoroll mentioned this pull request Mar 15, 2023

Roll Flutter Engine from b7297f025972 to 405c8513e4ca (7 revisions) flutter/flutter#122732

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2023

405c8513e [Impeller] Improve performance of CupertinoPicker with opac…

10b62b5

…ity peephole (flutter/engine#40101)

engine-flutter-autoroll mentioned this pull request Mar 15, 2023

Roll Flutter Engine from b7297f025972 to 9fc3246bff18 (8 revisions) flutter/flutter#122734

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2023

405c8513e [Impeller] Improve performance of CupertinoPicker with opac…

d032a75

…ity peephole (flutter/engine#40101)

engine-flutter-autoroll mentioned this pull request Mar 15, 2023

Roll Flutter Engine from b7297f025972 to 3ed9f1236578 (9 revisions) flutter/flutter#122736

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2023

405c8513e [Impeller] Improve performance of CupertinoPicker with opac…

8e69844

…ity peephole (flutter/engine#40101)

engine-flutter-autoroll mentioned this pull request Mar 15, 2023

Roll Flutter Engine from b7297f025972 to e143b309cc50 (10 revisions) flutter/flutter#122740

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2023

405c8513e [Impeller] Improve performance of CupertinoPicker with opac…

0d31e32

…ity peephole (flutter/engine#40101)

engine-flutter-autoroll mentioned this pull request Mar 15, 2023

Roll Flutter Engine from b7297f025972 to 086b1a022dfc (11 revisions) flutter/flutter#122744

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2023

405c8513e [Impeller] Improve performance of CupertinoPicker with opac…

2b3159d

…ity peephole (flutter/engine#40101)

engine-flutter-autoroll mentioned this pull request Mar 15, 2023

Roll Flutter Engine from b7297f025972 to ec151bf2cf72 (12 revisions) flutter/flutter#122746

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2023

405c8513e [Impeller] Improve performance of CupertinoPicker with opac…

7d28661

…ity peephole (flutter/engine#40101)

jonahwilliams mentioned this pull request Mar 17, 2023

[Impeller] CupertinoPicker very lag when scroll flutter/flutter#121476

Closed

		@@ -4,6 +4,7 @@

		#include "impeller/entity/contents/text_contents.h"

		#include <iostream>

[Impeller] Improve performance of CupertinoPicker with opacity peephole #40101

[Impeller] Improve performance of CupertinoPicker with opacity peephole #40101

Conversation

jonahwilliams commented Mar 7, 2023 • edited

Impeller Before

Impeller After

Skia

jonahwilliams commented Mar 7, 2023

Choose a reason for hiding this comment

bdero Mar 8, 2023 • edited

Choose a reason for hiding this comment

bdero Mar 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams Mar 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Mar 9, 2023

jonahwilliams commented Mar 10, 2023

jonahwilliams commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Mar 15, 2023

bdero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Mar 15, 2023

jonahwilliams commented Mar 7, 2023 •

edited

bdero Mar 8, 2023 •

edited

bdero Mar 8, 2023 •

edited

jonahwilliams Mar 7, 2023 •

edited