914617 - Use caching to speed up CSS clip-path when possible

Status: NEW

(Core :: Layout, enhancement)

Description

[Matthew N. [:MattN]]

Removing the new tab button clip-path accounted for "~10% of TART frame interval differences on my XP single-core VM with HWA off" (from bug 904924 comment 0).

Commenting out the remaining two clip-path additions for Australis shows more large gains[1] on this same system averaging about 12%.

Is there any way to cache some intermediate or rasterized state to make the clipping faster? The clip-path shape is pretty basic.

* svg:clipPath elements: https://mxr.mozilla.org/projects-central/source/ux/browser/base/content/browser.xul?rev=53f83c3fc0aa&mark=1110-1115#1109
* CSS references: https://mxr.mozilla.org/projects-central/source/ux/browser/themes/shared/tabs.inc.css?rev=306f68dadc04&mark=156,161#153

Let me know if this is not feasible.

[1] http://compare-talos.mattn.ca/breakdown.html?oldTestIds=130&newTestIds=132&testName=tart&osName=MattN-WinXP&server=graphs.mattn.ca

Comment 1

[Avi Halachmi (:avih)]

Would it be possible to disable the clip paths, or otherwise simplify them - during tab animations?

Comment 2

[:Gijs (he/him)]

(In reply to Avi Halachmi (:avih) from comment #1)
> Would it be possible to disable the clip paths, or otherwise simplify them -
> during tab animations?

AIUI, not easily, as they would cause the background to leak to the full size of the 'sides' of the tabs, making it look very ugly.

Comment 3

[Avi Halachmi (:avih)]

My bad, I thought the paths were for mouse-ovrer etc. Indeed we shouldn't disable/simplify visual paths.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

It's both - the clip-paths are used to clip the background shape, and to clip the mouse events.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We probably want to do this in Moz2D somehow.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, that's completely wrong. We currently use MaskLayers to accelerate rounded-rect clipping, and cache rasterized MaskLayers. We should extend that to handle clip-path too.

Comment 7

[Seth Fowler [:seth] [:s2h]]

I'll jump on this one.

Comment 8

[Matthew N. [:MattN]]

Hey Seth, 

How does this compare to bug 764299 in terms of the amount of work required? When do you think you could have a WIP patch to benchmark with? This patch should have an immediate improvement for Australis as-is whereas taking advantage of bug 764299 will require reworking Australis tabs again.

Comment 9

[Matthew N. [:MattN]]

Could anyone else help with estimates? This will likely fix our last Australis landing blocker (more data incoming from talos) but we don't know if we can count on this fix without time/effort estimates.

Thanks

Comment 10

[Matthew N. [:MattN]]

Updated compare-talos showing the gains from removing clip-path in order to measure its cost:
http://compare-talos.mattn.ca/?oldRevs=7cd2eb5814df&newRev=79189b7b6bf5&submit=true
It seems like this will also help ts_paint on XP quite a bit.

Comment 11

[Daniel Holbert [:dholbert]]

I don't know enough about our clip-path impl to respond usefully to your "needinfo" here. I think roc's probably the most likely to know how much work would be involved here. (so, good call needinfo'ing him as well)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is not all that much work but I'm notoriously bad at predicting actual schedules.

Comment 13

[Matthew N. [:MattN]]

Thanks roc.

Just an FYI that we are planning on going with the approach in bug 921038 for Australis tabs so we can take advantage of SVG caching from bug 764299 (thanks Seth!). That will unblock Australis on the perf issues since that should bring us to parity with m-c on (ts_paint, tpaint, and TART).

Based on what :jwatt said in bug 921673 comment 5 it seems like sticking with the approach on UX tip (with a CSS clip-path property) and getting this bug for caching would be more performant but bug 921038 will have to do for now.

In summary, this is a lower priority for Australis as we should be unblocked by bug 921038 for perf issues. If we're not the only reason to implement this and the cost is small then it's probably still nice to have since we still use clip-paths for the back and forward buttons but those are less perf-sensitive I would imagine.

Comment 14

[Jonathan Watt [:jwatt]]

Seth, when you implement this can you put it behind a bool pref svg.clipPath.cache, please? It's always useful to be able to measure the effect caching has on specific SVGs in future, and I'd particularly like to measure the effect vs. just path caching.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're planning to give this to an intern so I hope Seth isn't working on it :-)

Comment 16

[Seth Fowler [:seth] [:s2h]]

Glad you said something since I was just about to start working on it.

Is the intern starting on this in the near future? I get the impression from Australis folks that this is still somewhat of a priority.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to Seth Fowler [:seth] from comment #16)
> Glad you said something since I was just about to start working on it.
> 
> Is the intern starting on this in the near future? I get the impression from
> Australis folks that this is still somewhat of a priority.

I don't believe that's the case anymore.

Comment 18

[Seth Fowler [:seth] [:s2h]]

They said that to me during the rendering work week. It's possible things have changed since then.

Comment 19

[Matthew N. [:MattN]]

It is definitely still wanted for Australis to fix a usability regression (see bug 934915) but if we *have to* we can ship without this proper fix. It's no longer a hard blocker since we worked around the perf issue and thus caused the UX regression.

Comment 20

[Seth Fowler [:seth] [:s2h]]

So: should we still have the intern do this?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, I guess not. Go for it.

Comment 22

[Seth Fowler [:seth] [:s2h]]

OK, sounds good.

Comment 23

[Matthew N. [:MattN]]

Seth, any update? This may also help with the perf issues we're seeing in bug 990084 comment 5. At this point it might be a good intern project since Australis is already on 29.

Comment 24

[Jonathan Watt [:jwatt]]

Seth?