[Impeller] remove heap allocation of most geometry objects. #55677

jonahwilliams · 2024-10-05T20:11:25Z

Almost all geometry objects can now be stack allocated while rendering, instead of heap allocated - due to the removal of the EntityPass compositor. Clip geometries must still be heap allocated as the geometry objects are not yet stored in the display list, and the clips must be recorded for backdrop filter clip replay.

The canvas stores unique_ptrs to these geometries in a vector that it clears at the end of the frame. The Geometry factory methods were adjusted to return unique ptrs, and the Geometry class given a virtual dtor since we no longer rely on the special property of std::shared_ptr for memorizing the right dtor.

The ColorSourceContents and ClipContents class now hold onto a const ptr to the geometry object.

At some point in the future, we can rework the geometry object to be stored inline in the display list which will further simplify this code.

Part of flutter/flutter#142054

jonahwilliams · 2024-10-05T20:12:30Z

impeller/display_list/canvas.cc

  AddRenderEntityToCurrentPass(entity, false);
 }

+void Canvas::AddRenderEntityWithFiltersToCurrentPass(Entity& entity,


This is CreateContentsForGeometryWithFilters but as an instance method. It immediately renders the entity at the end, which guarantees its safe to provide with a stack allocated geometry object.

flutter-dashboard · 2024-10-06T00:02:01Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #55677 at sha b104b6c

chinmaygarde

Mostly just nits. lgtm

chinmaygarde · 2024-10-08T18:02:05Z

impeller/entity/geometry/cover_geometry.h

  CoverGeometry& operator=(const CoverGeometry&) = delete;
 };

-static_assert(std::is_trivially_destructible<CoverGeometry>::value);


A bit of a weird assertion to have in the first place TBH. Do we know why this was important? And presumably isn't now?

I added this when we were prototyping doing a placement new allocator, but that is made obsolete by the DisplayList alignment

chinmaygarde · 2024-10-08T18:02:19Z

impeller/entity/geometry/circle_geometry.cc

  FML_DCHECK(radius >= 0);
 }

+CircleGeometry::~CircleGeometry() {}


Nit: Default.

chinmaygarde · 2024-10-08T18:05:37Z

impeller/display_list/canvas.cc

+
+  entity.SetContents(std::move(contents_copy));
+  AddRenderEntityToCurrentPass(entity, reuse_depth);
+  return;


Nit: Unnecessary return.

chinmaygarde · 2024-10-08T18:06:32Z

impeller/display_list/canvas.cc

+    return;
+  }
+
+  std::shared_ptr<Contents> contents_copy = std::move(contents);


Maybe I'm missing something obvious but I can't tell why this var was copied.

contents is std::shared_ptr, but this gets assigned to so its std::shared_ptr. Same reason as the original code did this.

chinmaygarde · 2024-10-08T18:07:14Z

impeller/display_list/canvas.cc

+    }
+    if (paint.invert_colors) {
+      contents_copy =
+          WrapWithInvertColors(FilterInput::Make(contents_copy),


Can you move in the pointer here too?

chinmaygarde · 2024-10-08T18:08:05Z

impeller/entity/geometry/round_rect_geometry.cc

 RoundRectGeometry::RoundRectGeometry(const Rect& bounds, const Size& radii)
    : bounds_(bounds), radii_(radii) {}

+RoundRectGeometry::~RoundRectGeometry() {}


Nit: default

chinmaygarde · 2024-10-08T18:08:18Z

impeller/entity/geometry/rect_geometry.cc


 RectGeometry::RectGeometry(Rect rect) : rect_(rect) {}

+RectGeometry::~RectGeometry() {}


Nit: default

chinmaygarde · 2024-10-08T18:08:22Z

impeller/entity/geometry/point_field_geometry.cc

                                       bool round)
    : points_(std::move(points)), radius_(radius), round_(round) {}

+PointFieldGeometry::~PointFieldGeometry() {}


Nit: default

chinmaygarde · 2024-10-08T18:08:30Z

impeller/entity/geometry/line_geometry.cc

  FML_DCHECK(width >= 0);
 }

+LineGeometry::~LineGeometry() {}


Nit: default.

gaaclarke · 2024-10-08T20:01:57Z

impeller/entity/contents/filters/gaussian_blur_filter_contents.h

  const Vector2 sigma_ = Vector2(0.0, 0.0);
  const Entity::TileMode tile_mode_;
  const BlurStyle mask_blur_style_;
-  std::shared_ptr<Geometry> mask_geometry_;


This is eschewed by the C++ style guide: https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers

Who is the owner of this Geometry?

…flutter/engine#55677)

…156433) flutter/engine@167a42e...ea4a00f 2024-10-08 [email protected] [Impeller] remove heap allocation of most geometry objects. (flutter/engine#55677) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jonahwilliams · 2024-10-28T17:09:48Z

This improved dynamic path tessellation by about 0.5 ms:

https://flutter-flutter-perf.skia.org/e/?begin=1726254668&end=1730134927&queries=device_type%3DPixel_7_Pro%26sub_result%3D90th_percentile_frame_rasterizer_time_millis%26sub_result%3D99th_percentile_frame_rasterizer_time_millis%26sub_result%3Daverage_frame_rasterizer_time_millis%26sub_result%3Dworst_frame_rasterizer_time_millis%26test%3Ddynamic_path_tessellation_perf__timeline_summary&requestType=0&selected=commit%3D42863%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DPixel_7_Pro%252Cdevice_version%253Dnone%252Chost_type%253Dlinux%252Csub_result%253Daverage_frame_rasterizer_time_millis%252Ctest%253Ddynamic_path_tessellation_perf__timeline_summary%252C

…engine#55677) Almost all geometry objects can now be stack allocated while rendering, instead of heap allocated - due to the removal of the EntityPass compositor. Clip geometries must still be heap allocated as the geometry objects are not yet stored in the display list, and the clips must be recorded for backdrop filter clip replay. The canvas stores unique_ptrs to these geometries in a vector that it clears at the end of the frame. The Geometry factory methods were adjusted to return unique ptrs, and the Geometry class given a virtual dtor since we no longer rely on the special property of std::shared_ptr for memorizing the right dtor. The ColorSourceContents and ClipContents class now hold onto a const ptr to the geometry object. At some point in the future, we can rework the geometry object to be stored inline in the display list which will further simplify this code. Part of flutter#142054

jonahwilliams added 2 commits October 5, 2024 12:27

[Impeller] remove heap allocation of most geometry objects.

6bd7bc2

update testing.

721b405

github-actions bot added the e: impeller label Oct 5, 2024

jonahwilliams commented Oct 5, 2024

View reviewed changes

++

b104b6c

flutter-dashboard bot added the will affect goldens label Oct 6, 2024

jonahwilliams requested a review from chinmaygarde October 7, 2024 16:19

chinmaygarde approved these changes Oct 8, 2024

View reviewed changes

chinmay review.

4b9d22c

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 8, 2024

auto-submit bot merged commit ea4a00f into flutter:main Oct 8, 2024
30 checks passed

jonahwilliams deleted the remove_aiks_mask_blur branch October 8, 2024 19:55

gaaclarke reviewed Oct 8, 2024

View reviewed changes

engine-flutter-autoroll mentioned this pull request Oct 8, 2024

Roll Flutter Engine from 167a42e50fa2 to ea4a00f1c123 (1 revision) flutter/flutter#156433

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 8, 2024

ea4a00f1c [Impeller] remove heap allocation of most geometry objects. (…

8e18b7f

…flutter/engine#55677)

dkwingsmt mentioned this pull request Nov 20, 2024

Impeller playground no longer react to controllers flutter/flutter#159213

Closed


		RectGeometry::RectGeometry(Rect rect) : rect_(rect) {}

		RectGeometry::~RectGeometry() {}

[Impeller] remove heap allocation of most geometry objects. #55677

[Impeller] remove heap allocation of most geometry objects. #55677

Uh oh!

Conversation

jonahwilliams commented Oct 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Oct 6, 2024

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Oct 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants