Optimize setIsVolatile for Path

Internal: b/174239851

We had some performance issue that's related with SkPath::setIsVolatile before: #17889. That issue was closed because Skia introduced CCPR and it has a nice performance even without setIsVolatile.

Unfortunately, CCPR was later disabled #17889 (comment) and we're seeing this issue again.

In particular, here's a comparison of our cubic_bezier_perf__timeline_summary benchmark with and without setIsVolatile (using 8b201b9 and flutter-team-archive/engine@1482d9b):

	without setIsVolatile	with setIsVolatile
90th_percentile_frame_rasterizer_time_millis	24.108	13.95
99th_percentile_frame_rasterizer_time_millis	47.098	18.366

In addition to the speed improvement, setIsVolatile(true) should also improve the memory usage as Skia will no longer cache the volatile paths.

As path animations get more and more popular through Rive/Flare among our customers such as "customer: money (g3)" and "customer: dream (g3)", having setIsVolatile may become more and more beneficial (see, e.g., #57686).

@Hixie

CC @Hixie @chinmaygarde, we probably should revive flutter-team-archive/engine#5560.

Luckily, we have better ways to test memory now (thanks to @dnfield @terrylucas @iskakaushik @AlbertWang0116). To figure out whether we should setIsVolatile(true) by default, we could use our new tools to benchmark the CPU, GPU, and memory usage of this cubic_bezier_perf benchmark or some other more realistic Rive/Flare animations.

Also CC @pcsosinski @zoeyfan @mehmetf @jasonch99 to see how customers think about this issue, and what priority it has.

What if we just set the path as volatile until we actually draw it, after which we don't expect to modify it anymore?

@dnfield

@dnfield : setIsVolatile is kind of like setIsDrawingOnlyOnce which disables Skia to cache the path mask. So even if a path is not modified later, if that path is only drawn once, setIsVolatile still saves a lot of performance.

Once as in only to one SkPicture, or once as in the picture is drawn only once?

I'm not sure how a flutter developer would ever know whether to set this flag or not.

I'm not sure how a flutter developer would ever know whether to set this flag or not.

This is why we haven't exposed flags like this in the past. Unless we can solve this problem, we will definitely have people setting the flag at the wrong time and making things worse.

Once as in the picture is drawn only once.

I think we should set the default value to best fit most people, and leave an option for experts who will run benchmarks and figure out what's best for their use cases (e.g., Rive engineers).

Following Ian's earlier suggestion of introducing a new class, do you think the following class would be clear enough?

/// A [Path] that's expected to be drawn only once in the current frame,
/// and will never be drawn again in future frames.
///
/// This class aims purely at improving the performance of those one-time paths
/// by disabling some unnecessary caching. It has exactly the same visual result
/// and APIs as [Path].
class VolatilePath extends Path {
  ...
}

leave an option for experts who will run benchmarks and figure out what's best for their use cases (e.g., Rive engineers).

I 100% guarantee that such an API will be used by non-experts incorrectly. Here's a non-exhaustive list of ways it could happen:

Someone finds out about this API by looking at the API docs, and writes an unclear blog post with the title "One quick trick to improve your Flutter performance" and some sample code that people start cargo-culting into their app.
Someone includes this API in a YouTube video of "Top 10 Flutter APIs you didn't know about" and mentions that it's good for performance. Half a million viewers start using this API everywhere they can because "it's good for performance".
We work hard to hide the API so that only experts find it. Someone discovers its existence and tweets about it. Because it's an attractive "experts only" API and people like thinking they're experts, they start using it, even though it's inappropriate for them.
Someone uses it correctly in a library. Someone else forks that library and changes how it works, and now the library uses the API incorrectly.
Someone finds a library that uses it correctly, and cargo-cults its code into their app.

We should avoid having APIs that are traps like this. Otherwise, what will happen is five years from now we'll be making this API a no-op because people misuse it so much that it's better to just ignore calls to the API and use heuristics than it is to honor the API. (And we won't be able to remove it because that would break too many apps, so it'll just be a vestigial API.)

As an aside, introducing a new subclass for Path might turn every call to Path from a direct dispatch to a polymorphic dispatch, we should check with the VM folks.

@reed-at-google

I see. So it seems that our goal is to setIsVolatile internally using some heuristics, and not expose an API for developers?

The information needed for making the heuristic decision seems to be only available on the Skia level. @reed-at-google @bsalomon : can Skia set the path as volatile by default, and turn it to non-volatile once the path is rasterized multiple times? To preserve the old behavior, we could only enable that heuristics under some opt-in flag such as SkPath::setDynamicVolatility.

To @Hixie : sorry that I remember something wrong. Your suggestion was not about a new class, but a new constructor for Path. So it should be something like Path.makeVolatile and we shouldn't have the polymorphic dispatch problem?

Finally, we can always just setIsVolatile(true) for all paths in Flutter if it's at least better than setIsVolatile(false) for all paths in Flutter. Hence I'm renaming this issue's title from "Expose setIsVolatile for Path" to "Optimize setIsVolatile for Path".

I don't know enough about setIsVolatile to say what out goal should be, just that it's one of those mysterious APIs that I predict will get misused. :-)

Re the polymorphic dispatch thing, it's probably worth doing a one-off benchmark to see what the impact is. There's almost certainly areas of our framework where we fall prey to this problem today, might be a good place to look for optimizations in general if it's an actual thing.

I see. So if a path gets destructed in 1 frame, but ends up being recorded and re-rasterized in multiple frames with the same SkPicture, your approach will still mark it as volatile, and pay some potential performance cost? Do you have a number comparison like #58870 (comment) ?

Yes -

baseline

    "average_frame_build_time_millis": 2.2711369294605817,
    "90th_percentile_frame_build_time_millis": 3.21,
    "99th_percentile_frame_build_time_millis": 19.642,
    "worst_frame_build_time_millis": 60.696,
    "missed_frame_build_budget_count": 18,
    "average_frame_rasterizer_time_millis": 5.746145228215772,
    "90th_percentile_frame_rasterizer_time_millis": 10.647,
    "99th_percentile_frame_rasterizer_time_millis": 26.335,
    "worst_frame_rasterizer_time_millis": 87.355,
    "missed_frame_rasterizer_budget_count": 23,
    "frame_count": 1205,
    "frame_rasterizer_count": 1205,

after

    "average_frame_build_time_millis": 2.309207407407407,
    "90th_percentile_frame_build_time_millis": 3.059,
    "99th_percentile_frame_build_time_millis": 18.608,
    "worst_frame_build_time_millis": 54.183,
    "missed_frame_build_budget_count": 17,
    "average_frame_rasterizer_time_millis": 5.269529654036252,
    "90th_percentile_frame_rasterizer_time_millis": 9.842,
    "99th_percentile_frame_rasterizer_time_millis": 24.982,
    "worst_frame_rasterizer_time_millis": 60.98,
    "missed_frame_rasterizer_budget_count": 21,
    "frame_count": 1215,
    "frame_rasterizer_count": 1214,

And it substantially reduces graphics memory usage on the rive-app/example.

Cool! I wonder if you can also add a per-frame-tracing that counts how many paths are volatile and how many paths are non-volatile in flutter-team-archive/engine#22620? I think it might be useful to better understand the behavior and debug future performance changes.

Looks like PR was merged

Was reverted, should be relanded again soon though

Had to be reverted again - the implementation caused non-deterministic screen diff test behavior. Should be fixable.

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

Navigation Menu

Repository navigation

Optimize setIsVolatile for Path #58870

20 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Optimize setIsVolatile for Path #58870

Description

Activity

liyuqian commented on Jun 6, 2020

dnfield commented on Jun 6, 2020

liyuqian commented on Jun 6, 2020

dnfield commented on Jun 6, 2020

Hixie commented on Jun 6, 2020

liyuqian commented on Jun 8, 2020

Hixie commented on Jun 8, 2020

liyuqian commented on Jun 8, 2020

Hixie commented on Jun 8, 2020

20 remaining items

liyuqian commented on Nov 19, 2020

dnfield commented on Nov 19, 2020

liyuqian commented on Nov 21, 2020

thecalamiity commented on Dec 12, 2020

dnfield commented on Dec 12, 2020

dnfield commented on Dec 21, 2020

github-actions commented on Aug 8, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions