[Impeller] Reduce allocations for polyline generation #47837

dnfield · 2023-11-08T23:38:20Z

Before this patch, polyline generation did lots of small allocations over and over again.

Now, Path::CreatePolyline takes a std::vector<Point> that the caller can reserve such that it won't likely need to be resized. The tessellator has been updated to hold a thread_local point buffer that gets reused and avoids lots of intra-frame allocation. This improves the profiles seen in flutter/flutter#138004, specifically around the FillPointsForPolyline time

This very significantly improves the benchmarking data in geometry_benchmarks.

The first commit in this patch slightly improves the benchmarks by reducing some allocations, but not enough of them.

jonahwilliams · 2023-11-08T23:54:08Z

impeller/entity/geometry/stroke_path_geometry.cc

  VertexBufferBuilder<VS::PerVertexData> vtx_builder;
-  auto polyline = path.CreatePolyline(scale);
+  std::vector<Point> point_buffer;
+  point_buffer.reserve(512);


document magic numbers? what is this from?

I made it up!

Shall I add a comment saying I made this up?

Yes please!

That way we remember its not a Chesterton's Fence situation in the future.

jonahwilliams

Overall LGTM with question on stroke tessellator.

bdero

LGTM. Allocation gainz.

bdero · 2023-11-08T23:45:18Z

impeller/tessellator/tessellator.cc

-    const Path::Polyline& polyline,
-    const BuilderCallback& callback) const {
+Tessellator::Result Tessellator::Tessellate(const Path& path,
+                                            Scalar scale,


Nit: Tolerance?

gaaclarke

Just gave it a quick look. While I'm on the road it's hard to focus.

gaaclarke · 2023-11-09T00:05:57Z

impeller/geometry/path.h

    /// Points in the polyline, which may represent multiple contours specified
    /// by indices in |breaks|.
-    std::vector<Point> points;
+    std::vector<Point>& points;


This makes me a bit nervous, storing a pointer as an instance method. Potentially you could instead recycle these and have a unique_ptr around them. So you pass it ownership when you create it. Then you take ownership, then you can reuse it with the next Polyline if you want.

I reworked the patch to use unique_ptr and have a callback fired in the dtor to reclaim it for code paths that need it. I think this cleans up the API a little bit, and it keeps the benchmark happy.

gaaclarke · 2023-11-09T00:06:11Z

impeller/tessellator/tessellator.cc


 namespace impeller {

+FML_THREAD_LOCAL std::vector<Point> tls_point_buffer;


TLS isn't free, do we actually use the tessellator from multiple threads?

AFAICT the underlying libtess2 TESStesselator held by the impeller::Tessellator is not thread safe.

I'd suggest moving the reusable point buffer into the impeller::Tessellator and documenting that impeller::Tessellator is not thread safe.

That's fair, I was trying to be more defensive here but don't need to be. I'll make this just a regular ivar on impeller::Tessellator.

gaaclarke

This is an improvement. It is a bit awkward but I don't have a better suggestion. I was thinking of an explicit take for the points, but I can see where the function has it's benefits. Looks good, but I think we should tidy up the tests and put them in the proper location.

gaaclarke · 2023-11-09T21:26:13Z

impeller/entity/geometry/stroke_path_geometry.cc

-                                             middle_handle, middle)
-                              .CreatePolyline(scale);
+        std::vector<Point> arc_points;
+        CubicPathComponent(start_offset, start_handle, middle_handle, middle)


Can the reserve size be calculated here?

These are small and shouldn't result in lots of reallocation. I'd feel more comfortable having a benchmark showing this before trying to place a number in it.

I ask because I'm turning on a linter for this in #47868. I don't think it will flag this though since it can't trivially know what the reserve size is.

Reserving incorrect sizes can make things worse. If we don't know let's use the standard vec reallocation startegy

gaaclarke · 2023-11-09T21:26:31Z

impeller/entity/geometry/stroke_path_geometry.cc

        vtx.position = position - orientation;
        vtx_builder.AppendVertex(vtx);
-        for (const auto& point : arc.CreatePolyline(scale)) {
+        std::vector<Point> arc_points;


Same deal. This shouldn't typically result in a large number of points.

gaaclarke · 2023-11-09T21:30:27Z

impeller/geometry/path.h

+    using PointBufferPointer = std::unique_ptr<std::vector<Point>>;
+    using ReclaimPointBuffer = std::function<void(PointBufferPointer)>;


I feel like these using statements make the code harder to follow.

I was getting tired of typing out std::unique_ptr<std::vector<Point>> in a large number of places.

If you want to keep these I think you show follow the conventions (inconsistently) used elsewhere in the codebase, the "Ptr" and "Callback" suffixes.

gaaclarke · 2023-11-09T21:48:20Z

impeller/geometry/path.h

+
+    /// The buffer will be cleared and returned at the destruction of this
+    /// polyline.
+    Polyline(PointBufferPointer point_buffer, ReclaimPointBuffer reclaim);


This is a bit awkward but given c++'s lack of lifetimes I think this is an improvement and maybe the best we can do. I think you should move the polyline tests to a path_unittests.cc. Having the tests in a nonstandard location makes it hard to verify coverage.

I think if we want to break up geometry_unittests.cc maybe let's do it in a separate PR.

A followup PR sounds good to me. It's a bit easier to review this one if the tests are in the proper place but I can understand wanting to separate refactoring and functional changes.

jason-simmons · 2023-11-09T22:00:11Z

impeller/tessellator/tessellator.cc

+  point_buffer_->clear();
+  auto polyline =
+      path.CreatePolyline(tolerance, std::move(point_buffer_),
+                          [&](Path::Polyline::PointBufferPointer point_buffer) {


The lambda capture can be reduced to [this]

jason-simmons · 2023-11-09T22:01:06Z

impeller/tessellator/tessellator.cc

+    // Some polygons will not self close and an additional triangle
+    // must be inserted, others will self close and we need to avoid
+    // inserting an extra triangle.
+    if ((*polyline.points)[end - 1] == (*polyline.points)[start]) {


Consider adding a method to Polyline that returns a reference to the point at a given index

gaaclarke

lgtm

gaaclarke · 2023-11-09T22:53:23Z

impeller/geometry/geometry_unittests.cc

+              });
+}
+
+TEST(GeometryTest, PolylineFailsWithNullptrBuffer) {


Thanks for adding these. A nullptr test for the reclaim callback would be nice too.

auto-submit · 2023-11-09T23:28:12Z

auto label is removed for flutter/engine/47837, due to - The status or check suite Linux linux_android_aot_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-11-10T00:18:06Z

auto label is removed for flutter/engine/47837, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ter/engine#47837)

dnfield · 2023-11-10T23:29:21Z

Up is good here: https://flutter-engine-perf.skia.org/e/?queries=test%3DBM_Polyline_cubic_polyline%26test%3DBM_Polyline_quad_polyline&selected=commit%3D38051%26name%3D%252Cconfig%253Ddefault%252Ccpu_scaling_enabled%253Dfalse%252Cexecutable%253D_b_s_w_ir_cache_builder_src_out_host_release_geometry_benchmarks%252Clibrary_build_type%253Drelease%252Cmhz_per_cpu%253D2200%252Cnum_cpus%253D8%252Csub_result%253DTotalPointCount%252Ctest%253DBM_Polyline_quad_polyline%252Cunit%253Dns%252C

We calculated more points using less CPU time.

…ter/engine#47837)

…138266) flutter/engine@9d8a112...00db306 2023-11-10 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Bump minSdk to 19 for Android tests" (flutter/engine#47935) 2023-11-10 [email protected] Roll Dart SDK from 91c4a92a64ea to 370145bbbd4f (1 revision) (flutter/engine#47930) 2023-11-10 [email protected] Roll Skia from 2c43bf002b7f to 96ce4d6f433d (3 revisions) (flutter/engine#47931) 2023-11-10 [email protected] [Impeller] implement Canvas::DrawLine to tesselate lines directly (flutter/engine#47846) 2023-11-10 [email protected] Roll Skia from d06840545bff to 2c43bf002b7f (1 revision) (flutter/engine#47928) 2023-11-10 [email protected] Bump minSdk to 19 for Android tests (flutter/engine#47686) 2023-11-10 [email protected] [Impeller] Reduce allocations for polyline generation (flutter/engine#47837) 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],[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

dnfield added 4 commits November 8, 2023 10:03

1

1bc0c4d

[Impeller] Reduce allocations in polyline generation

ebfaabb

merge

84d2e20

fixes for tests, minor cleanup/rename

a92b757

dnfield requested a review from jonahwilliams November 8, 2023 23:38

github-actions bot added the e: impeller label Nov 8, 2023

dnfield added 2 commits November 8, 2023 15:42

docs

aa280ec

one more reserve

cd60b9f

jonahwilliams reviewed Nov 8, 2023

View reviewed changes

jonahwilliams approved these changes Nov 8, 2023

View reviewed changes

bdero approved these changes Nov 8, 2023

View reviewed changes

gaaclarke reviewed Nov 9, 2023

View reviewed changes

chinmaygarde assigned dnfield Nov 9, 2023

No TLS, unique_ptr for vector with callback

07eee60

gaaclarke suggested changes Nov 9, 2023

View reviewed changes

jason-simmons reviewed Nov 9, 2023

View reviewed changes

dnfield added 3 commits November 9, 2023 14:18

more review

761be29

merge

4d9fe4e

names

720a0ee

jason-simmons approved these changes Nov 9, 2023

View reviewed changes

tests

615f8fd

dnfield added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Nov 9, 2023

gaaclarke approved these changes Nov 9, 2023

View reviewed changes

lint

71afaa0

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2023

lint

da16afe

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2023

dnfield added 2 commits November 9, 2023 20:11

Merge branch 'main' into alloc

d9a0ab9

Merge branch 'main' into alloc

eaa07be

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2023

auto-submit bot merged commit 1ca461b into flutter:main Nov 10, 2023

dnfield deleted the alloc branch November 10, 2023 21:35

engine-flutter-autoroll mentioned this pull request Nov 10, 2023

Roll Flutter Engine from 9d8a1125640d to 36f62055aa5c (2 revisions) flutter/flutter#138255

Closed

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

1ca461b78 [Impeller] Reduce allocations for polyline generation (flut…

ceb33cc

…ter/engine#47837)

engine-flutter-autoroll mentioned this pull request Nov 10, 2023

Roll Flutter Engine from 9d8a1125640d to 68ce6bf410e6 (3 revisions) flutter/flutter#138256

Closed

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

1ca461b78 [Impeller] Reduce allocations for polyline generation (flut…

5166546

…ter/engine#47837)

engine-flutter-autoroll mentioned this pull request Nov 10, 2023

Roll Flutter Engine from 9d8a1125640d to ce613ce95593 (5 revisions) flutter/flutter#138263

Closed

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

1ca461b78 [Impeller] Reduce allocations for polyline generation (flut…

7388686

…ter/engine#47837)

engine-flutter-autoroll mentioned this pull request Nov 10, 2023

Roll Flutter Engine from 9d8a1125640d to 28d9146eb5d6 (6 revisions) flutter/flutter#138264

Closed

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

1ca461b78 [Impeller] Reduce allocations for polyline generation (flut…

53ea02f

…ter/engine#47837)

engine-flutter-autoroll mentioned this pull request Nov 10, 2023

Roll Flutter Engine from 9d8a1125640d to 00db306f6f7b (7 revisions) flutter/flutter#138266

Merged

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

1ca461b78 [Impeller] Reduce allocations for polyline generation (flut…

f80e551

…ter/engine#47837)


		namespace impeller {

		FML_THREAD_LOCAL std::vector<Point> tls_point_buffer;

		using PointBufferPointer = std::unique_ptr<std::vector<Point>>;
		using ReclaimPointBuffer = std::function<void(PointBufferPointer)>;

[Impeller] Reduce allocations for polyline generation #47837

[Impeller] Reduce allocations for polyline generation #47837

Uh oh!

Conversation

dnfield commented Nov 8, 2023

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment