Use caching to speed up CSS clip-path when possible

ASSIGNED
Assigned to
(NeedInfo from)

Status

()

Core
Layout
--
enhancement
ASSIGNED
4 years ago
6 months ago

People

(Reporter: MattN, Assigned: seth, NeedInfo)

Tracking

(Blocks: 3 bugs, {perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P4][Australis:M-], URL)

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

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

Comment 2

4 years ago
(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

4 years ago
My bad, I thought the paths were for mouse-ovrer etc. Indeed we shouldn't disable/simplify visual paths.
It's both - the clip-paths are used to clip the background shape, and to clip the mouse events.
We probably want to do this in Moz2D somehow.
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.
(Assignee)

Comment 7

4 years ago
I'll jump on this one.
Assignee: nobody → seth
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.
Status: NEW → ASSIGNED
Flags: needinfo?(seth)
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
Flags: needinfo?(roc)
Flags: needinfo?(dholbert)
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.
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)
Flags: needinfo?(dholbert)
This is not all that much work but I'm notoriously bad at predicting actual schedules.
Flags: needinfo?(roc)
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.
Whiteboard: [Australis:P1][Australis:M?] → [Australis:P4][Australis:M-]
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.
We're planning to give this to an intern so I hope Seth isn't working on it :-)
Assignee: seth → nobody
(Assignee)

Comment 16

4 years ago
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.
(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.
(Assignee)

Comment 18

4 years ago
They said that to me during the rendering work week. It's possible things have changed since then.
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 20

4 years ago
So: should we still have the intern do this?
Flags: needinfo?(roc)
No, I guess not. Go for it.
Assignee: nobody → seth
Flags: needinfo?(roc)
(Assignee)

Comment 22

4 years ago
OK, sounds good.
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.
You need to log in before you can comment on or make changes to this bug.