[Impeller] Cache Polyline and tessellation for Path #44390

knopp · 2023-08-04T16:01:47Z

This is a follow-up PR for #44324.

Results for the static_path_tessellation macrobenchmark on iPhone 13 Pro (A15):

Renderer	average_frame_build_time_millis	worst_frame_build_time_millis
Impeller - Cache Disabled	0.47708099688473554	1.369
Impeller - Cache Enabled	0.49306597222222226	1.596
Skia - No Raster Cache	0.4640437500000002	1.309

Renderer	average_frame_rasterizer_time_millis	worst_frame_rasterizer_time_millis
Impeller - Cache Disabled	6.347621527777779	6.632
Impeller - Cache Enabled	0.6700383275261321	0.93
Skia - No Raster Cache	0.5126250000000006	0.973

I had to run the perf test with background thread spinning (see flutter/flutter#131837) otherwise the variant with cache enabled actually regressed in average_frame_build_time_millis because iOS let the CPU run at lower speed.

After #44248 Paths are now retained by entity pass created from UI canvas, so we can cache the polyline, bounds and tessellation result per path:

Path and Polyline are made internally reference counted. This is to ensure predictable behavior regarding their instances.
Path will cache Polyline and bounds.
Polyline stores cached tessellation data.

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jonahwilliams

See #44248

Once dispatching is removed and the UI thread talks directly to Impeller, there shouldn't be continual re-conversions of static paths

knopp · 2023-08-04T16:30:57Z

See #44248

Once dispatching is removed and the UI thread talks directly to Impeller, there shouldn't be continual re-conversions of static paths

That doesn't seem to be part of the linked PR though? Is it planned after? That would require changing CanvasPath to hold impeller path.

EDIT: Actually it would require DisplayList to store impeller path instead of SkPath.

jonahwilliams · 2023-08-04T16:36:51Z

Right, we'll still convert Skia -> Impeller. But I think that we'll likely end up reusing the same Impeller pictures (provided the picture layers are stable) which means substantially less dispatching.

Longer term we will eventually switch from SkPath to Impeller or DL path but that has runtime implications.

knopp · 2023-08-04T20:51:08Z

Right, we'll still convert Skia -> Impeller. But I think that we'll likely end up reusing the same Impeller pictures (provided the picture layers are stable) which means substantially less dispatching.

Longer term we will eventually switch from SkPath to Impeller or DL path but that has runtime implications.

My bad. I only noticed it once I tried it. It turns out that now entities are created by canvas on UI thread and will retain the Path. This is great!

knopp · 2023-08-04T21:05:26Z

I nuked everything related to caching of paths and just kept the part where path caches polyline, bounds and tessellation.

jonahwilliams · 2023-08-05T01:17:16Z

At a high level i think sort of approach is good, but I'll start looking into the details next week. Only caveat is making sure we measure when the cache doesn't work: flutter/flutter#131837 (comment)

jonahwilliams · 2023-08-13T20:34:50Z

Currently we'll need to reland the aiks canvas stuff.

flar · 2023-08-15T00:26:07Z

That doesn't seem to be part of the linked PR though? Is it planned after? That would require changing CanvasPath to hold impeller path.

EDIT: Actually it would require DisplayList to store impeller path instead of SkPath

That PR might help, but also I'm currently creating geometry classes that will use a DlPath instead of an SkPath. The path to better interoperation between Dart->DL->Impeller can also be achieved there. For now the path stuff is stubbed out in the new code (it just forwards to an SkPath, but eventually it will fill in with its own implementation that can favor Skia backend or Impeller backend, or be configurable). Minimally I plan to add a UniqueID feature that could be useful for caching.

There is no PR for the geometry stuff as I'm still working it through, but the work in progress is in my repo: https://github.com/flar/engine/tree/dl-geometry-classes/display_list/geometry

flar · 2023-08-15T00:27:25Z

See also the discussion as to what to do with DlPath here: flutter/flutter#119308

knopp · 2023-08-15T07:37:40Z

UniqueId (like generationId) sure helps, but I think ideally we would want to tie the tessellation data and polyline to the path instance itself, so we don't need to deal with eviction strategy.

jonahwilliams · 2023-08-24T22:59:31Z

@dnfield is currently working on relanding his aikscanvas patch, and after that lands then we can revisit this. As I look at more android profiles with impeller enabled I'm convinced we simply need to do this to work well on Android.

jonahwilliams · 2023-08-31T06:19:05Z

DLAiksCanvas has relanded. There are a few remaining issues like #45313 , but if that patch survives a bit longer then we can probably start landing stuff that depends on it

jonahwilliams

Mostly just questions about changes to the structure of Paths to start.

jonahwilliams · 2023-08-31T06:30:30Z

impeller/geometry/path.cc

@@ -11,13 +11,13 @@

 namespace impeller {

-Path::Path() {
+Path::Path() : state_(std::make_shared<State>()) {


So today we have Path and PathBuilder. Both are mutable right now, but we could probably simplify this implementation a lot by making Path only internally mutable. So if we knew that only PathBuilder was adding the components, we don't need to invalidate the cache ever in path methods.

It's invalidated in mutator methods. Are you suggesting removing the mutator methods from path completely? I'm wondering if there are plans later to have a TrackedPath backed by impeller path. In which case it will probably need to remain mutable.

The mutator methods are mostly used by pathbuilder. pathbuilder, doesn't need to be constantly reseting the caches, so maybe restrict the mutation via some friend class shennanigans?

The mutator methods are used in geometry_unittests.cc at least.

On second look those could probably easily be replaced by builder.

jonahwilliams · 2023-08-31T06:34:06Z

impeller/geometry/path.h


 private:
  friend class PathBuilder;

-  void SetConvexity(Convexity value);
+  void SetConvexity(Convexity value) { state_->SetConvexity(value); }


I think its pretty confusing to have two different classes called state in the same header. Why do we need to move all of the path state into this object?

Because path caches polyline and polyline caches tesselation data. With this in place we now care about what a path instance actually is. So the alternatives would be to make path non copyable and use shared_ptrs everywhere, or make path non copyable, have one owner and pass by reference everywhere (that would become problematic the moment we want to pass it to std::function for example).

Making the path state internally reference seemed like the least disruptive change. But it's possible least disruptive is not what the objective is :)

I don't understand what the value is of having two separate caches. Seems like Polyline data is almost always immediately passed to either Tessellate or Tessellate convex. And anything that invalidates the tessellated data always invalidates the polyline too. Can use just cache the tessellation output?

jonahwilliams · 2023-08-31T06:34:49Z

impeller/geometry/path.h

-    ComponentIndexPair(ComponentType a_type, size_t a_index)
-        : type(a_type), index(a_index) {}
+    mutable std::optional<std::optional<Rect>> cached_bounding_box_;
+    mutable std::optional<Polyline> cached_polyline_;


This is a separate cache from the tessellation cache, right?

Path instance caches polyline and bounding box. Polyline instance caches tessellation data.

dnfield · 2023-08-31T20:16:30Z

@knopp - WDYT about an API that would instead let applications (and the framework) explicitly prepare a path for repeated use?

In Impeller this would result in pre-tessellating the path, in Skia I'm not sure (maybe it skips the volatile path tracker and just marks it as non-volatile for example?).

I think the main concern I have with this change is that it will benefit some applications but if they make what seems like a small change they'll suddenly fall off a performance cliff - and it willr esult in developers trying to figure out what "tricks" to use to benefit from caching instead of being able to just explicitly say "here's a path I want to draw repeatedly, please keep using it".

jonahwilliams · 2023-08-31T20:18:23Z

Applications will not be able to use that API effectively in most cases. The CTM can change without rebuilding or repainting render objects.

jonahwilliams · 2023-08-31T23:43:00Z

If you want I can send a PR to make path externally immutable next week and clean up the test cases. I should have used PathBuilder anyway for those, but I was being sloppy.

knopp · 2023-09-01T16:59:40Z

@dnfield, this PR is mostly to address a possibly common scenario of SVG icons, where the paths are unlikely to change. My concern though here is that now the cache is tied to entity, which will be rebuilt with every new display list produced. Originally the cache was tied to a SkPath, referenced by canvas path. So the paths stayed same, even with new display list produced. But with AIKS canvas the cache is tied to specific entity, which means repaint boundaries and layers will affect the cache. I think I might have been too eager to remove the original caching code from the PR. I'll need to play with this a little bit.

I also don't think this should result in new APIs. Ideally this should be consider a stop-gap measure. Right now even with caching the end result slower than skia (their tesselation with middle-out triangulation seem to be really, really fast).

@jonahwilliams, I don't know if it's going to be a problem for future, but for now making the path immutable would make sense to me.

chinmaygarde · 2023-09-21T20:14:37Z

This is blocked on at least @dnfield change.

knopp · 2023-09-28T20:29:43Z

I'm putting this as WIP. I think this definitely needs to be done as long as we're using software tessellation, but because this PR depends on the entity retaining the path, i'm concerned that the cache may be invalidated during in cases where the picture layer gets repainted even though all used SkPaths are same.

zanderso · 2023-11-02T20:23:20Z

From Engine PR triage: I'm having trouble following the thread above. This PR seems to have been dependent on some other work landing. Is it time to pick this back up?

jonahwilliams · 2023-11-02T20:28:49Z

The work that this was blocked on has not relanded. I think (@dnfield correct if wrong) that work is still blocked by #46275 (comment)

knopp · 2023-11-02T21:07:54Z

There are two issues here - the PR that this depended on got reverted, and I'm not convinced that the solution here is robust enough. I believe that with this approach in some scenarios the cache can be invalidated even when the original SkPath does not change. The very first iteration used SkPath generationId as key which I think is more reliable.

This PR has also gotten quite stale, so I'll close it for now.

github-actions bot added the e: impeller label Aug 4, 2023

knopp added the Work in progress (WIP) Not ready (yet) for review! label Aug 4, 2023

knopp mentioned this pull request Aug 4, 2023

WIP: [impeller] Add tessellation cache #44324

Closed

8 tasks

knopp force-pushed the add_path_cache branch 3 times, most recently from 3f7b095 to 0b1bec0 Compare August 4, 2023 16:20

jonahwilliams suggested changes Aug 4, 2023

View reviewed changes

knopp force-pushed the add_path_cache branch from 0b1bec0 to 4379ffc Compare August 4, 2023 16:33

knopp force-pushed the add_path_cache branch 6 times, most recently from c07f142 to b34753b Compare August 4, 2023 19:24

chinmaygarde assigned knopp Aug 4, 2023

This comment was marked as off-topic.

Sign in to view

flutter-dashboard bot added the will affect goldens label Aug 4, 2023

Cache polyline and tesselation on Path

95d7e84

knopp changed the title ~~WIP: [Impeller] Add path cache~~ WIP: [Impeller] Cache Polyline and tessellation for path Aug 4, 2023

knopp force-pushed the add_path_cache branch from b34753b to 95d7e84 Compare August 4, 2023 21:02

knopp changed the title ~~WIP: [Impeller] Cache Polyline and tessellation for path~~ [Impeller] Cache Polyline and tessellation for path Aug 4, 2023

knopp removed will affect goldens Work in progress (WIP) Not ready (yet) for review! labels Aug 4, 2023

knopp requested a review from dnfield August 4, 2023 21:02

knopp changed the title ~~[Impeller] Cache Polyline and tessellation for path~~ [Impeller] Cache Polyline and tessellation for Path Aug 4, 2023

jonahwilliams reviewed Aug 31, 2023

View reviewed changes

knopp added the Work in progress (WIP) Not ready (yet) for review! label Sep 28, 2023

knopp closed this Nov 2, 2023

[Impeller] Cache Polyline and tessellation for Path #44390

[Impeller] Cache Polyline and tessellation for Path #44390

Uh oh!

Conversation

knopp commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

knopp commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Aug 4, 2023

Uh oh!

This comment was marked as off-topic.

knopp commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knopp commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Aug 5, 2023

Uh oh!

jonahwilliams commented Aug 13, 2023

Uh oh!

flar commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flar commented Aug 15, 2023

Uh oh!

knopp commented Aug 15, 2023

Uh oh!

jonahwilliams commented Aug 24, 2023

Uh oh!

jonahwilliams commented Aug 31, 2023

Uh oh!

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

dnfield commented Aug 31, 2023

Uh oh!

jonahwilliams commented Aug 31, 2023

Uh oh!

jonahwilliams commented Aug 31, 2023

Uh oh!

knopp commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde commented Sep 21, 2023

Uh oh!

knopp commented Sep 28, 2023

Uh oh!

zanderso commented Nov 2, 2023

Uh oh!

jonahwilliams commented Nov 2, 2023

Uh oh!

knopp commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

knopp commented Aug 4, 2023 •

edited

Loading

knopp commented Aug 4, 2023 •

edited

Loading

knopp commented Aug 4, 2023 •

edited

Loading

knopp commented Aug 4, 2023 •

edited

Loading

flar commented Aug 15, 2023 •

edited

Loading

knopp commented Sep 1, 2023 •

edited

Loading

knopp commented Nov 2, 2023 •

edited

Loading