[Impeller] generate stroke vertices into point arena. #56390

jonahwilliams · 2024-11-05T22:28:44Z

Heap allocation is extremely expensive on Android.

We can speed up the stroke tessellation by allocation a large arena and using that to write vertices. If the vertices would overflow, we switch to a dynamically allocated vector.

jonahwilliams · 2024-11-06T02:15:50Z

impeller/entity/geometry/stroke_path_geometry.cc


-  arc.ToLinearPathComponents(scale, [&vtx_builder, &vtx, forward_normal,
-                                     position](const Point& point) {
+  Scalar line_count = std::ceilf(ComputeCubicSubdivisions(scale, arc));


From profiling, calling ToLinearPathComponents with a closure triggers a heap allocation on Android.

Bummer, we could get away with it by eliminating all captures and passing in a pointer to something on the stack. This isn't a request to make that change, but I think it's better to keep the abstraction since it's used in multiple locations.

std::tuple<Foo, Bar, Baz> info = {foo, bar, baz}; arc.ToLinearPathComponents<decltype(info)>( scale, info, [](const Point"& point, const decltype(info)& info) { auto vtx = info.position + point; //... });

jonahwilliams · 2024-11-06T02:15:56Z

impeller/entity/geometry/stroke_path_geometry.cc

-        vtx.position = position + (-point * direction).Reflect(middle_normal);
-        vtx_builder.AppendVertex(vtx.position);
-      });
+  CubicPathComponent arc(start_offset, start_handle, middle_handle, middle);


Same here,

From profiling, calling ToLinearPathComponents with a closure triggers a heap allocation on Android.

jonahwilliams · 2024-11-06T02:16:11Z

impeller/entity/geometry/stroke_path_geometry.cc


 namespace {

-template <typename VertexWriter>


Removed template code as its unused, we only have a single implementation.

gaaclarke

LGTM, code looks good, I just have some nits on documentation and a suggestion on how to keep the abstraction we already had (up to you if you want to do that though).

gaaclarke · 2024-11-06T17:01:14Z

impeller/entity/entity_unittests.cc

+  Path path = builder.TakePath();
+  auto geom = Geometry::MakeStrokePath(path, /*stroke_width=*/10);
+
+  ContentContext content_context(GetContext(), nullptr);


Suggested change

ContentContext content_context(GetContext(), nullptr);

ContentContext content_context(GetContext(), /*typographer_context=*/nullptr);

gaaclarke · 2024-11-06T17:03:02Z

impeller/entity/entity_unittests.cc

+      content_context.GetContext()->GetResourceAllocator());
+
+  auto render_target =
+      allocator.CreateOffscreen(*content_context.GetContext(), {10, 10}, 1);


Suggested change

allocator.CreateOffscreen(*content_context.GetContext(), {10, 10}, 1);

allocator.CreateOffscreen(*content_context.GetContext(), /*size=*/{10, 10}, /*mip_count=*/1);

gaaclarke · 2024-11-06T17:07:10Z

impeller/entity/entity_unittests.cc

+  EXPECT_NE(*(written_data + kPointArenaSize - 2), Point(0, 0));
+  EXPECT_NE(*(written_data + kPointArenaSize - 1), Point(0, 0));
+  EXPECT_NE(*(written_data + kPointArenaSize), Point(0, 0));
+  EXPECT_NE(*(written_data + kPointArenaSize + 1), Point(0, 0));
+  EXPECT_NE(*(written_data + kPointArenaSize + 2), Point(0, 0));
+  EXPECT_NE(*(written_data + kPointArenaSize + 3), Point(0, 0));


Can we switch these to EXPECT_EQ assertions? Not being Point(0,0) leaves a lot of possibilities.

Since this is stroke tessellation the generated points are floating point values like 43.22132 ...

I could do a CLOSE_TO or something like that though. As long as they are in the ballpark

gaaclarke · 2024-11-06T17:32:40Z

impeller/entity/geometry/stroke_path_geometry.cc

+    }
+  }
+
+  size_t GetUsedSize() const { return offset_; }


It's worth documenting what this is supposed to return in a docstring since it's up to the client of this class to do the correct math with this and the oversized buffer's size.

Actually, we could probably change this to just return offset_ + oversized_buffer_.size() to avoid the weirdness.

We need both numbers to split up the memcp correctly

gaaclarke · 2024-11-06T17:34:10Z

impeller/entity/geometry/stroke_path_geometry.cc

+class PositionWriter {
+ public:
+  explicit PositionWriter(std::vector<Point>& points)
+      : points_(points), oversized_(0) {}


Suggested change

: points_(points), oversized_(0) {}

: points_(points), oversized_() {}

gaaclarke · 2024-11-06T17:35:22Z

impeller/entity/geometry/stroke_path_geometry.cc

+class PositionWriter {
+ public:
+  explicit PositionWriter(std::vector<Point>& points)
+      : points_(points), oversized_(0) {}


Suggested change

: points_(points), oversized_(0) {}

: points_(points), oversized_(0) {

FML_DCHECK(points.size() >= kPointArenaSize);

}

gaaclarke · 2024-11-06T17:42:16Z

impeller/entity/geometry/stroke_path_geometry.cc


-  arc.ToLinearPathComponents(scale, [&vtx_builder, &vtx, forward_normal,
-                                     position](const Point& point) {
+  Scalar line_count = std::ceilf(ComputeCubicSubdivisions(scale, arc));


Bummer, we could get away with it by eliminating all captures and passing in a pointer to something on the stack. This isn't a request to make that change, but I think it's better to keep the abstraction since it's used in multiple locations.

std::tuple<Foo, Bar, Baz> info = {foo, bar, baz}; arc.ToLinearPathComponents<decltype(info)>( scale, info, [](const Point"& point, const decltype(info)& info) { auto vtx = info.position + point; //... });

gaaclarke · 2024-11-06T17:59:47Z

impeller/entity/geometry/stroke_path_geometry.cc

+                          .transform = entity.GetShaderTransform(pass),
+                          .mode = GeometryResult::Mode::kPreventOverdraw};
+  }
+  FML_LOG(ERROR) << "Oversized";


Stray log? It's worth keeping a comment here.

gaaclarke · 2024-11-06T18:02:19Z

impeller/entity/geometry/stroke_path_geometry.cc

+  const std::vector<Point>& oversized_data =
+      position_writer.GetOveriszedBuffer();
+  BufferView buffer_view = host_buffer.Emplace(
+      nullptr,


Suggested change

nullptr,

/*buffer=*/nullptr,

gaaclarke

lgtm, thanks!

…ter/engine#56390)

…158295) flutter/engine@58ac1da...b36ca33 2024-11-06 [email protected] [skwasm] Fix empty backdrop drawing. (flutter/engine#56385) 2024-11-06 [email protected] [Impeller] generate stroke vertices into point arena. (flutter/engine#56390) 2024-11-06 [email protected] [Impeller] Do not capture the temporary ImpellerMapping struct pointer when storing release callbacks in libImpeller (flutter/engine#56411) 2024-11-06 6844906[email protected] Roll ICU from 9408c6fd4a39 to 4239b1559d11 (2 revisions) (flutter/engine#56407) 2024-11-06 [email protected] iOS,macOS: Add Obj-C cflags to all Obj-C targets (flutter/engine#56386) 2024-11-06 [email protected] Roll Skia from afaed8923682 to cf33c4e96e81 (5 revisions) (flutter/engine#56408) 2024-11-06 [email protected] Manual roll Dart SDK from 1c1d0420539f to d456f613465a (6 revisions) (flutter/engine#56406) 2024-11-06 [email protected] [Impeller] Avoid errors due to triangle fans usage on Molten. (flutter/engine#56321) 2024-11-06 [email protected] Roll Skia from b4df8dda7ffc to afaed8923682 (14 revisions) (flutter/engine#56404) 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-11-07T16:08:29Z

Numbers look good:

https://flutter-engine-perf.skia.org/e/?keys=X32514c31fc24cb3328f96455630936ff&selected=commit%3D43577%26name%3D%252Cconfig%253Ddefault%252Ccpu_scaling_enabled%253Dfalse%252Cexecutable%253D_b_s_w_ir_cache_builder_src_out_ci_host_release_benchmarks_geometry_benchmarks%252Clibrary_build_type%253Drelease%252Cmhz_per_cpu%253D2200%252Cnum_cpus%253D8%252Csub_result%253DTotalPointCount%252Ctest%253DBM_StrokePolyline_stroke_RRect_Butt_Round%252Cunit%253Dns%252C&xbaroffset=43577

…#56390) Heap allocation is extremely expensive on Android. We can speed up the stroke tessellation by allocation a large arena and using that to write vertices. If the vertices would overflow, we switch to a dynamically allocated vector.

[Impeller] generate vertices into point arena.

fed2d65

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

jonahwilliams added 4 commits November 5, 2024 15:19

fix return of temporary object.

6ff9748

Add unit tests and fix branch condition.

8333074

fix includes by moving to entity unit tests.

8de5631

use constant for arena size.

783c093

jonahwilliams marked this pull request as ready for review November 6, 2024 02:13

jonahwilliams commented Nov 6, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

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

fix round cap generation.

b03c4e5

jonahwilliams removed the will affect goldens label Nov 6, 2024

jonahwilliams changed the title ~~[Impeller] generate vertices into point arena.~~ [Impeller] generate stroke vertices into point arena. Nov 6, 2024

jonahwilliams requested a review from gaaclarke November 6, 2024 15:43

gaaclarke reviewed Nov 6, 2024

View reviewed changes

aaron review.

dd2bf07

gaaclarke approved these changes Nov 6, 2024

View reviewed changes

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

auto-submit bot merged commit 00c22da into flutter:main Nov 6, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Nov 6, 2024

Roll Flutter Engine from 58ac1dadd69d to b36ca3319825 (9 revisions) flutter/flutter#158295

Merged

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

00c22da5b [Impeller] generate stroke vertices into point arena. (flut…

0658510

…ter/engine#56390)

	ContentContext content_context(GetContext(), nullptr);
	ContentContext content_context(GetContext(), /typographer_context=/nullptr);

	allocator.CreateOffscreen(*content_context.GetContext(), {10, 10}, 1);
	allocator.CreateOffscreen(content_context.GetContext(), /size=/{10, 10}, /mip_count=*/1);

	: points_(points), oversized_(0) {}
	: points_(points), oversized_() {}

[Impeller] generate stroke vertices into point arena. #56390

[Impeller] generate stroke vertices into point arena. #56390

Uh oh!

Conversation

jonahwilliams commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

This comment was marked as outdated.

gaaclarke 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!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams commented Nov 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonahwilliams commented Nov 5, 2024 •

edited

Loading