938742 - Add padding to top and bottom of new tab button icon to make layout generate a single rect

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

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

The science of how layout breaks things down into rects is still a bit of a mystery to me, but through the magic of Jeff, we've determined that animating two things of varying heights results in multiple rects, and that multiple rects are slower than a single rect.

We create multiple rects when we open a tab because the new tab button icon (which has no chrome around it) is smaller than the tab that's opening to the left of it. Because it's vertically centered, we break into 3 strips - the top of the tab (6px tall), the middle of the tab + the new tab icon (19px tall), and then the bottom of the tab (6px tall).

Because the middle strip is wider than the top and bottom strips, we have 3 rects. And that's slow.

The solution is to make the new tab icon as tall as the tab in the eyes of the layout engine. We add 6px above and below the new tab icon, and voila, single rect. jrmuizel and I tested this locally, and this does result in us going from 3 to 1 rect.

This should result in a nice perf win, at least on Windows. Not sure how it'll work for Linux or OS X yet, but I'll write the patch for those as well.

Comment 1

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

According to Jeff, there should be perf wins on at least OS X as well - but they might not be detectable.

Comment 2

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

Attachment: Patch v1

Funnily enough, we were already doing something like this for the OS X new tab button for what's likely a different reason, but I added the comment referring to this bug anyhow.

I'll push to try next (with bug 934860 backed out to best approximate the impact on TART on landing).

Comment 3

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

Both of these builds have bug 934860 backed out.

UX baseline: https://tbpl.mozilla.org/?tree=Try&rev=fd7407dccf60
UX + patch: https://tbpl.mozilla.org/?tree=Try&rev=545e4b8f4ff0

compare-talos: http://compare-talos.mattn.ca/?oldRevs=fd7407dccf60&newRev=545e4b8f4ff0&server=graphs.mozilla.org&submit=true

Comment 4

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

Hm, this patch actually seemed to make things a little worse on Windows 7...

Comment 5

[Matt Woodrow (:mattwoodrow)]

When you said this takes us from 3 rects to 1, was that just for a specific tab, or was that the total number of rects during a (representative) paint while running TART?

If the total rects painted contains something else (so that we still have more than one total), then we're going to hit the D2D slow path. Simplifying the rects is just making us paint more pixels.

This is *not* the case with bug 934860 applied, since then we essentially run a separate paint for each rect. This always means we avoid the slow path, but instead we can end up paying some overhead costs multiple times. In this situation, we are probably better off simplifying the rects to reduce this overhead, but it's kind of a tradeoff.

Comment 6

[Avi Halachmi (:avih)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> In this situation, we are probably better off simplifying the rects to
> reduce this overhead, but it's kind of a tradeoff.

Is this something the fx-team can aim for in advance? I.e. have some guidelines on how to plan the layout such that it performs well?

I'd think we'd prefer that the layout code just manages to optimize everything and we don't have to tweak the layout for performance, but is this realistic?

Or maybe the truth lays somewhere in between where some layout tweaks would be required occasionally to improve perf?

Comment 7

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

Whichever the case, jrmuizel has expressed to me that it's probably not worth trying to investigate this any further now that bug 934860 is getting backed out. Closing as WONTFIX unless someone really wants to dive in here.