1484780 - Cache the gfx::Path built from StyleSVGPath to avoid rebuilding it too many times

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Boris Chiou [:boris]]

Bug 1429298 implemented |offset-path: path()|, but we build the gfx::Path and compute the point at length too many times. Those operations are expensive, so it's worth to cache them.

Comment 1

[Boris Chiou [:boris]]

Hi Nical,

I'm trying to cache the gfx::Path [1] to avoid the cost of flattening it again each time. This may be useful because, in most cases, the path is the same, and we just update the offset-distance (i.e. change the position along the path). However, I'm not sure where is a suitable place to store the cached value. It seems nsDisplayTransform may be reconstructed during the animation, so would you suggest cache the gfx::Path in nsFrame? For now, it seems we use a const nsIFrame during resolving motion path (and other transform-related properties), so maybe need to make the cached variable mutable. Not sure if it is a good idea. Perhaps you have a better idea for this? Thanks.

[1] https://searchfox.org/mozilla-central/rev/7531325c8660cfa61bf71725f83501028178cbb9/layout/base/nsLayoutUtils.cpp#10326-10327

Comment 2

[Boris Chiou [:boris]]

Oh. Perhaps we should build the path when initializing the frame.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

If we plan to run motion path properties on the compositor, I think we probably don't need to cache it.

Comment 4

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

If we plan to run motion path properties on the compositor, I think we probably don't need to cache it.

I use this test, https://codepen.io/BorisChiou/pen/xpLbov, to check the performance. For now, it's unacceptable on Firefox. After applying my hack which caches the gfx::Path in nsIFrame, it looks much better, but still not enough (, though my local test uses the debug build).

So IMO, it's worth to cache the path, and even on the compositor? We probably need to find a suitable place to cache it on the compositor as well.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

To be clear what I meant is that we don't need to cache it on the main thread.

Comment 6

[Nicolas Silva [:nical]]

Caching the path makes sense (wherever it is used, both on the compositor and the main thread if need be). I'm not very familiar with most of the code under the layout/ directory but Matt Woodrow and Markus Stange among others would have more educated opinions on the topic.

If you need to further optimize this, note that we currently build a flattened version of the whole path, and then traverse it to find a specific position along. We could:

• Not build the flattened path and have the flattening code just measure distances instead of generating a path.
• Build an array of "check points" for example which would give us for a given path event where we are (position and advancement along the path), so that we can fast track to the last check-point before the advancement we are looking for and then only do the flattening approximation from there (this only makes sense if you are already caching the path).
  In case you are interested in these optimizations, I have something that resembles the first one in a side project of mine: https://github.com/nical/lyon/blob/master/algorithms/src/walk.rs (I haven't implemented the second one). Until now path flattening was not performance sensitive in Gecko because it was only used to position text along a path so I never bothered to port that over to Gecko.
  Anyway, caching is certainly more important than these two optimizations cand you can add them on top if need be.

Comment 7

[Boris Chiou [:boris]]

Nice suggestion for more optimization. Ya. at least do cache on the compositor thread. For main thread, it's still possible to run motion on the main thread, e.g. if we set transform property !important, so should be great to also cache it on the main thread.

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1484780 - Cache gfx::Path to avoid building and flattening path at each restyle cycle.

In most cases, we run an animation on an object by changing its
offset-distance/offset-rotate, but keep its offset-path the same.
Building and flattening the path is sometime expensive, especially for
large path, so caching it makes sense to us and have a significant
performance improvement.

This is a temporary patch for discussion. I'm not sure whether storing
the path data in nsIFrame as mutable variables is acceptable or not.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I am still skeptical it's worth caching it on the main-thread, at least we should run the properties on the compositor first. Then if there are still many cases that the properties cannot be run on the compositor, caching it on the main-thread is one of options to solve the situation.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #7)

Nice suggestion for more optimization. Ya. at least do cache on the compositor thread. For main thread, it's still possible to run motion on the main thread, e.g. if we set transform property !important, so should be great to also cache it on the main thread.

Note that, !import transform property shouldn't stop running motion path properties on the compositor, I think. I maybe missing something though.

Comment 11

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)

Note that, !import transform property shouldn't stop running motion path properties on the compositor, I think. I maybe missing something though.

Just like this test [1] and the current implementation [2], we have to merge transform property, individual transform properties, and motion path transform properties all together to get the final transform matrix. This means we have to make sure all of them are on the same thread. If any of them cannot run on the compositor, we have to fall back to main thread.

[1] https://searchfox.org/mozilla-central/rev/e18057a9613ffda06dfd3640209ca234ed7dc37d/dom/animation/test/chrome/test_running_on_compositor.html#1118-1143
[2] https://searchfox.org/mozilla-central/rev/e18057a9613ffda06dfd3640209ca234ed7dc37d/dom/animation/KeyframeEffect.cpp#1504-1520

Comment 12

[Hiroyuki Ikezoe (:hiro)]

This is our current limitation for current transform like properties. We want to do the same limitation on motion path properties? Even so, such kind of cases is not common cases, so we should do optimizations for most cases first.

Comment 13

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)

This is our current limitation for current transform like properties. We want to do the same limitation on motion path properties? Even so, such kind of cases is not common cases, so we should do optimizations for most cases first.

Thanks, Hiro. I see. Anyway, I should work on Bug 1429305 first, so we can focus on the common cases. :)

Comment 14

[Boris Chiou [:boris]]

Just notice one thing:
Even we run the animations on the compositor, we still call nsIFrame::GetTransformMatrix() by nsLayoutUtils::GetTransformToAncestor() during the animation. (No idea for now.) This means we rebuild the path at each tick on the main thread even we can run the animation on the compositor.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0ddb98d498d
Cache gfx::Path to avoid building and flattening path at each restyle cycle. r=heycam

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c0ddb98d498d