[Impeller] store all path point data in single buffer. #47896

jonahwilliams · 2023-11-10T06:26:26Z

Path objects are expensive to allocate, create, move, et cetera because of the large number of internal heap allocations performed. This change combines the linear, quad, and cubic buffers into a single point buffer that stores all points/control points inline.

The existing component buffer stores the index to the first point in the buffer, and the type field determines how many points are read.

Benchmarked in https://flutter-engine-perf.skia.org/e/?queries=test%3DBM_Polyline_cubic_polyline%26test%3DBM_Polyline_quad_polyline&selected=commit%3D38035%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

flutter/flutter#138004

jonahwilliams · 2023-11-10T16:39:37Z

We could still combine the contour data into the path data with a little more work. Some additional work would be to add a reasonable "reserve" functionality for use when converting from SkPath.

…into combine_components

jonahwilliams · 2023-11-14T21:12:26Z

Friendly ping @dnfield

jonahwilliams · 2023-11-15T21:04:14Z

impeller/display_list/skia_conversions.cc

@@ -57,6 +57,7 @@ Path ToPath(const SkPath& path, Point shift) {

  PathBuilder builder;
  PathData data;
+  builder.Reserve(path.countPoints() + 16, path.countVerbs() + 16);


since our storage more closely aligns with the SkPath, we can guestimate more easily...

I guess we're assuming that stays true.

I'd probably leave a 1-line explaining how you came to these numbers for the next shmuck.

Added a line here

matanlurey · 2023-11-15T21:21:29Z

impeller/display_list/skia_conversions.cc

@@ -57,6 +57,7 @@ Path ToPath(const SkPath& path, Point shift) {

  PathBuilder builder;
  PathData data;
+  builder.Reserve(path.countPoints() + 16, path.countVerbs() + 16);


I guess we're assuming that stays true.

I'd probably leave a 1-line explaining how you came to these numbers for the next shmuck.

matanlurey · 2023-11-15T21:23:31Z

impeller/geometry/geometry_unittests.cc

Maybe it's time to bite the bullet and create a new file (path_unittests.cc)?

Moved to path_unittests.cc

This came up in another review I was on too. In impeller there is some non-standard organization for tests. It makes a big difference for reviewing if when a file is changed the associated test file is changed too. Thanks for cleaning this up.

gaaclarke

LGTM, I think the union would clean things up a bit.

gaaclarke · 2023-11-16T17:00:18Z

impeller/geometry/path.cc

-        break;
-    }
-    currentIndex++;
+  for (auto i = 0u; i < points_.size(); i++) {


FYI this is where having a linear algrebra library would be nice. This sort of operation would be 10x faster if it was vectorized.

This isn't really a bottleneck though!

gaaclarke · 2023-11-16T17:03:26Z

impeller/geometry/path.cc

@@ -246,11 +258,14 @@ Path::Polyline Path::CreatePolyline(
    const auto& component = components_[component_i];
    switch (component.type) {
      case ComponentType::kLinear:
-        return &linears_[component.index];
+        return reinterpret_cast<const LinearPathComponent*>(


nit: you could use a union here instead of having these casts.

A union would require us to align storage to the largest component size, effectively padding out linear components 4x. The cast approach means that we can store points inline/continuously, though we have to rely on the secondary vector of verbs to determine the correct offsets. This is approach is closer to what Skia uses and I think we should too (partially because it makes point/verb reserve work nicely), and partially because its closer to the format we need for compute anyway.

At least, I think it would? Is that right or wrong. 😆

Yea, could be. I'm not familiar enough with the code to know if there are less items in the vector to account for the different type. If the counts are the same then you're right.

You could make your own cast with template specialization

case ComponentType::kLinear: return point_cast<ComponentType::kLinear>(&points_[component.index]);

That's a wee bit cleaner. I don't have another easy solution.

I'm going to punt on this for now.

…tter/engine#47896)

…138568) flutter/engine@22baa83...094a338 2023-11-16 [email protected] Roll Skia from 297049bbfc0d to 4c099aaa259f (5 revisions) (flutter/engine#48138) 2023-11-16 [email protected] Roll Dart SDK from b512191e9612 to 906f23c1cb20 (1 revision) (flutter/engine#48136) 2023-11-16 [email protected] Disable the `runIfNot` clauses in `.ci.yaml`, as they are unsafe. (flutter/engine#48132) 2023-11-16 [email protected] Fix race condition in Unobstructed Platform View Scenario tests (flutter/engine#48096) 2023-11-16 [email protected] [Impeller] store all path point data in single buffer. (flutter/engine#47896) 2023-11-16 [email protected] Roll Skia from b9ead4140f84 to 297049bbfc0d (2 revisions) (flutter/engine#48133) 2023-11-16 [email protected] [macOS] Replace pasteboard mock with fake (flutter/engine#48110) 2023-11-16 [email protected] [Impeller] Fix issue where the lock was not re-acquired when the wait exits on CV. (flutter/engine#48104) 2023-11-16 [email protected] [Impeller] Create a drawable backed TextureMTL. (flutter/engine#48049) 2023-11-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make `flow/embedded_views.h` compatible with `.clang_tidy`." (flutter/engine#48130) 2023-11-16 [email protected] [macOS] Replace fixture subclasses with usings (flutter/engine#48111) 2023-11-16 [email protected] Make `flow/embedded_views.h` compatible with `.clang_tidy`. (flutter/engine#47994) 2023-11-16 [email protected] Make `flow/...` compatible with `.clang_tidy`. (flutter/engine#47995) 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

[Impeller] just crazy enough to work.

f485ad2

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

jonahwilliams added 2 commits November 9, 2023 23:09

++

25e1900

++

9085c49

jonahwilliams changed the title ~~[Impeller] just crazy enough to work.~~ [Impeller] store all path point data in single buffer. Nov 10, 2023

++

70cc0c3

jonahwilliams added 2 commits November 10, 2023 10:35

++

44c3f3b

Merge branch 'main' into combine_components

ec7478d

jonahwilliams marked this pull request as ready for review November 10, 2023 19:19

jonahwilliams requested a review from dnfield November 10, 2023 19:19

jonahwilliams added 2 commits November 11, 2023 08:06

Merge branch 'main' of github.com:flutter/engine into combine_components

f525594

Merge branch 'combine_components' of github.com:jonahwilliams/engine …

7600750

…into combine_components

jonahwilliams requested review from matanlurey and gaaclarke November 15, 2023 21:02

jonahwilliams commented Nov 15, 2023

View reviewed changes

matanlurey approved these changes Nov 16, 2023

View reviewed changes

jonahwilliams added 2 commits November 16, 2023 08:19

Merge branch 'main' of github.com:flutter/engine into combine_components

e1c9f32

matanl review.

7caf0c4

gaaclarke reviewed Nov 16, 2023

View reviewed changes

++

fedb8a2

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

auto-submit bot merged commit f7e5ae4 into flutter:main Nov 16, 2023

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

Roll Flutter Engine from 22baa83db63b to 13cdf933d641 (10 revisions) flutter/flutter#138563

Closed

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

f7e5ae481 [Impeller] store all path point data in single buffer. (flu…

71f84d7

…tter/engine#47896)

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

Roll Flutter Engine from 22baa83db63b to 094a3383a406 (13 revisions) flutter/flutter#138568

Merged

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

f7e5ae481 [Impeller] store all path point data in single buffer. (flu…

764003b

…tter/engine#47896)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] store all path point data in single buffer. #47896

[Impeller] store all path point data in single buffer. #47896

jonahwilliams commented Nov 10, 2023 •

edited

Loading

jonahwilliams commented Nov 10, 2023

jonahwilliams commented Nov 14, 2023

jonahwilliams Nov 15, 2023

matanlurey Nov 15, 2023

jonahwilliams Nov 16, 2023

matanlurey Nov 15, 2023

matanlurey Nov 15, 2023

jonahwilliams Nov 16, 2023

gaaclarke Nov 16, 2023

gaaclarke left a comment

gaaclarke Nov 16, 2023

jonahwilliams Nov 16, 2023

gaaclarke Nov 16, 2023

jonahwilliams Nov 16, 2023

jonahwilliams Nov 16, 2023

gaaclarke Nov 16, 2023

jonahwilliams Nov 16, 2023

[Impeller] store all path point data in single buffer. #47896

[Impeller] store all path point data in single buffer. #47896

Conversation

jonahwilliams commented Nov 10, 2023 • edited Loading

jonahwilliams commented Nov 10, 2023

jonahwilliams commented Nov 14, 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

gaaclarke 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Nov 10, 2023 •

edited

Loading