353757 - New theme landing caused a Ts regression on trunk

Status: RESOLVED INCOMPLETE

(Firefox :: General, defect)

Description

[Adam Guthrie]

This was probably less-noticeable on branch since the theme went in in pieces, but when it went in in one big chunk on trunk, it's hit on Ts was fairly obvious.

The numbers are roughly a 3% increase on Linux, and a 5% increase on Windows. 

Some numbers...
argo: 897ms -> 933ms
argo test: 2138ms -> 2203ms
gais test: 1672ms -> 1750ms

Comment 1

[Boris Zbarsky [:bzbarsky]]

For what it's worth, on the 1.8 branch pacifica-vm (Windows) shows a 16% Ts regression over the last 2 months, and sparky (Linux) shows a 4% Ts regression over the same time period.  Not sure what else has been landing there, though.  See http://build-graphs.mozilla.org/graph/query.cgi?tbox=pacifica-vm_mozilla_1_8_branch&testname=startup&autoscale=1&size=&units=ms&ltype=&points=&showpoint=2006%3A09%3A21%3A22%3A33%3A40%2C719&avg=1&days=60 and http://build-graphs.mozilla.org/graph/query.cgi?tbox=sparky.office.mozilla.org_mozilla_1_8_branch&testname=startup&autoscale=1&size=&units=ms&ltype=&points=&showpoint=2006%3A09%3A21%3A22%3A22%3A29%2C2724&avg=1&days=60

This should probably be considered for blocking firefox3.  Certainly any Gecko checkin which caused a startup regression this big would be blocking.

Up to the Firefox 2 drivers what they want to do for Firefox 2 at this point, if anything.

Comment 2

[Adam Guthrie]

There's an interesting little dip in Ts (about 1 or 2%) on sparky between 2006-09-11 and 2006-09-14 which seems to be related to theme changes.

http://build-graphs.mozilla.org/graph/query.cgi?tbox=sparky.office.mozilla.org_mozilla_1_8_branch&testname=startup&autoscale=1&days=14&units=ms&ltype=&points=&showpoint=2006%3A09%3A21%3A22%3A22%3A29%2C2724&avg=1&size=1

Dip down: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-10+21&maxdate=2006-09-11+06&cvsroot=%2Fcvsroot
Rise: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-13+21&maxdate=2006-09-14+06&cvsroot=%2Fcvsroot

Comment 3

[Asaf Romano (gone)]

Attachment: Might help

I cannot test winstripe patches right now, I will get Gavin to test this later today.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: minor optimizations

This is a slightly tweaked subset of the changes from Mano's "might help" patch that I just landed. I'm not sure that it will make a noticeable difference, since I think the addition of images is what made the most difference, but I landed it anyways.

Comment 5

[Asaf Romano (gone)]

Not /really/ mine.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I suspect the images are the #1 issue as well.

That said, looking at the CSS being modified I highly urge everyone involved to read <http://developer.mozilla.org/en/docs/Writing_Efficient_CSS> and in particular <http://developer.mozilla.org/en/docs/Writing_Efficient_CSS#Avoid_the_descendant_selector.21>.

I'm looking at selectors like ".searchbar-popup .open-engine-manager", "#wrapper-search-container #searchbar html|*.textbox-input", "#wrapper-personal-bookmarks .toolbarpaletteitem-box", "toolbarpaletteitem[place="toolbar"] .bookmarks-toolbar-customize", etc. etc.

Maybe some of these are unavoidable....  But I'm somehow having doubts that all of them are.

Comment 7

[Boris Zbarsky [:bzbarsky]]

That said, I'd expect the CSS stuff to affect Txul as well as Ts, and I don't see much of a Txul jump on argo.  But it looks like a lot of those rules were already there...  And all the Windows Txul tests are useless (> 5% noise), so who knows what's happening there.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> Maybe some of these are unavoidable....  But I'm somehow having doubts that all
> of them are.

Most of them are only avoidable by chaining together many child selectors to get through the anonymous content of an XBL binding (like the searchbar html:input selector in my patch). I could spend time optimizing those all to avoid the descendant selector as much as possible, but I'm not sure that it would be worth it, given that most of those rules are not new, and that the images seem to be the main cause of the slowdown. It would also make the theme much more fragile to XUL structure changes, and make it harder to maintain by increasing its size and complexity.

I'm not sure what to do about the images, really. We should probably try backing out the changes made in bug 347616 and bug 348138, though I'm not sure how hard it would be to isolate those changes at this point.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Are there a bunch of images that could be using -moz-image-region but aren't?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> Are there a bunch of images that could be using -moz-image-region but aren't?

I think bug 351764 was preventing us from using -moz-image-region for the endcaps and autocomplete dropdown, Pam or Myk would know more.

Comment 11

[Dão Gottwald [:dao]]

All the navigation toolbar icons (and maybe others) are completely alpha-blended. I think the old ones had semi-transparent borders only. I could imagine that this matters, but then I don't even know what the tests look like.
At least this could be changed easily, given the fact that people complained about faint/weakly saturated icons: make them opaque.


(In reply to comment #9)
> Are there a bunch of images that could be using -moz-image-region but aren't?

I see the following groups (again, I don't know if they matter for the tests):

alltabs-box-overflow-end-bkgnd.png
alltabs-box-overflow-end-bkgnd-animate.png
alltabs-box-overflow-end-bkgnd-hover.png

alltabs-box-overflow-start-bkgnd.png
alltabs-box-overflow-start-bkgnd-animate.png
alltabs-box-overflow-start-bkgnd-hover.png

alltabs-box-end-bkgnd.png
alltabs-box-end-bkgnd-hover.png

alltabs-box-start-bkgnd.png
alltabs-box-start-bkgnd-hover.png

tab-arrow-end-bkgnd-animate.png
tab-arrow-end-bkgnd-disabled.png
tab-arrow-end-bkgnd-enabled.png (matches alltabs-box-end-bkgnd.png)
tab-arrow-end-bkgnd-hover.png (matches alltabs-box-end-bkgnd-hover.png)

tab-arrow-start-bkgnd-animate.png
tab-arrow-start-bkgnd-disabled.png
tab-arrow-start-bkgnd-enabled.png (matches alltabs-box-start-bkgnd.png)
tab-arrow-start-bkgnd-hover.png (matches alltabs-box-start-bkgnd-hover.png)

tab-left.png
tab-left-bkgnd.png
tab-left-hover.png (matches tab-left.png)

tab-middle.png
tab-middle-bkgnd.png
tab-middle-hover.png (matches tab-left.png)

tab-right.png
tab-right-bkgnd.png
tab-right-hover.png (matches tab-left.png)

tab-arrow-end.png and tab-arrow-start.png could be combined, too.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> tab-middle.png
> tab-middle-bkgnd.png
> tab-middle-hover.png (matches tab-left.png)
> 
> tab-right.png
> tab-right-bkgnd.png
> tab-right-hover.png (matches tab-left.png)

should be:

tab-middle.png
tab-middle-bkgnd.png
tab-middle-hover.png (matches tab-middle.png)

tab-right.png
tab-right-bkgnd.png
tab-right-hover.png (matches tab-right.png)

Comment 13

[Pam Greene]

(In reply to comment #10)
> I think bug 351764 was preventing us from using -moz-image-region for the
> endcaps and autocomplete dropdown, Pam or Myk would know more.

In winstripe, each of those four buttons (autocomplete dropdown, Go, Search, and search engine selection) uses five images, three of which use -moz-image-region and two of which can't because of bug 351764.

In pinstripe, each button uses a single image, and of course there are lots of other differences between the themes (in tabs, other buttons, etc.) as well. Did Mac Ts take a hit too?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Did Mac Ts take a hit too?

The graph at <http://build-graphs.mozilla.org/graph/query.cgi?tbox=xserve03.build.mozilla.org_fx-trunk&testname=startup&autoscale=1&size=&units=ms&ltype=&points=&showpoint=2006%3A09%3A22%3A08%3A21%3A53%2C1476&avg=1&days=4> is kinda useless (what happened to the older data?  this picks up right around when the theme landed).  The numbers on tbox indicate a <1% Ts regression.  Maybe.  The numbers fluctuate some from 161-3 to 170-ish, and I can't tell how those correspond to theme stuff.

Comment 15

[Mike Connor [:mconnor]]

As a note, I think the old Mac tabstrip was also using images, so Mac perf changes should be minor at worst.

Taking for now, I think we can probably just lump all of the tab images listed in comment 11 into a single image.  I"m sure there will be some wasted space, but since all, or nearly all of those are loaded early, its worth a bit of wasted space.

Comment 16

[(not reading, please use [e-mail] instead)]

about mac vs non-mac, on mac, by default, we don't always show the tab bar by default.  (So if there is only one tab, we won't see it.)

give that the tab bar has lots of new images, could this explain why the Mac Ts was less affected than the non-Mac Ts?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We don't show the tab strip by default with one tab across platforms.  But on first start we show two tabs -- although I don't think the Ts test would hit that case.

However, depending on how the tab strip is hidden, we might be loading the images for it anyway.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Attempt to profile startup -- ZIP file containing jprof.html

Interestingly, I don't see much about images in this profile... This is my local Firefox trunk with the new theme, vanilla profile.  I basically started jprof without JP_DEFER and stopped it inside the startup-test onload handler where we do the timing.

But jprof isn't very good for measuring things like I/O waits, I suspect.

Comment 19

[Mike Schroepfer]

Attachment: Tabs combined into one image

Comment 20

[Mike Schroepfer]

Attachment: -moz-image-region for attached combined tabs.png

In case it helps - these two attachments are the tab images combined into one big image along with the corresponding moz-image-region rects.   This was generated from a script so if you need it tiled differently or want a different set of images that can be done very quickly.

Comment 21

[Asaf Romano (gone)]

We really need that for winstripe first.

Comment 22

[Mike Schroepfer]

Ahh roger.  Coming up...

Comment 23

[Mike Schroepfer]

Attachment: tabs.ping for winstripe

Comment 24

[Mike Schroepfer]

Attachment: -moz-image-region for attached combined winstripe tabs.png

Winstripe version.  Let me know if this works/you need it tiled differently.

Comment 25

[Asaf Romano (gone)]

Working on this...

Comment 26

[Asaf Romano (gone)]

Comment on attachment 239847 [details]
-moz-image-region for attached combined winstripe tabs.png

OK, I need a new combined image with the following images only:

>alltabs-box-end-bkgnd-hover.png
>alltabs-box-overflow-end-bkgnd-hover.png
>alltabs-box-overflow-start-bkgnd-hover.png
>alltabs-box-start-bkgnd.png
>alltabs-box-end-bkgnd.png
>alltabs-box-overflow-end-bkgnd.png
>alltabs-box-overflow-start-bkgnd.png
>alltabs-box-overflow-end-bkgnd-animate.png
>alltabs-box-overflow-start-bkgnd-animate.png
>alltabs-box-start-bkgnd-hover.png
>tab-arrow-end-bkgnd-animate.png
>tab-arrow-end-bkgnd-disabled.png
>tab-arrow-end-bkgnd-enabled.png
>tab-arrow-end-bkgnd-hover.png
>tab-arrow-end.png
>tab-arrow-start-bkgnd-animate.png
>tab-arrow-start-bkgnd-disabled.png
>tab-arrow-start-bkgnd-enabled.png
>tab-arrow-start-bkgnd-hover.png
>tab-arrow-start.png
>tab-left-bkgnd.png
>tab-left-hover.png
>tab-left.png
>tab-right-bkgnd.png
>tab-right-hover.png
>tab-right.png

Comment 27

[Mike Schroepfer]

Attachment: Updated tabs list - removed tab-middle and tabstrip-bottom.png tabbrowser-tabs-bkgnd.png

Comment 28

[Mike Schroepfer]

Attachment: moz-image-region for updated tab images.

Comment 29

[Mike Schroepfer]

Attachment: Updated tabs list - removed tab-middle and tabstrip-bottom.png tabbrowser-tabs-bkgnd.png (without 1 pixel spacing)

Comment 30

[Mike Schroepfer]

Attachment: moz-image-region for updated tab images (without 1 pixel spacing)

Comment 31

[Mike Schroepfer]

Attachment: Python script to stitch images together

Here's the script I hacked up earlier today to do this in case you need to make further mods.   Only requirements are python + Python Imaging Library (http://www.pythonware.com/products/pil/)

I'm not a big python hacker so I'm sure there are some humorous python gaffes here.  But it does the trick.

Comment 32

[Mike Schroepfer]

Attachment: moz-image-region where schrep learns that the rects are (top, right, bottom, left) not (left, top, right, bottom)

Comment 33

[Mike Schroepfer]

Attachment: Script with correct moz-image-region output

Comment 34

[Dão Gottwald [:dao]]

(In reply to comment #29)
> Created an attachment (id=239860) [edit]

Those are still the same, so I don't think they need to be 2 times in there:

tab-arrow-end-bkgnd-enabled.png / alltabs-box-end-bkgnd.png
tab-arrow-end-bkgnd-hover.png / alltabs-box-end-bkgnd-hover.png
tab-arrow-start-bkgnd-enabled.png / alltabs-box-start-bkgnd.png
tab-arrow-start-bkgnd-hover.png / alltabs-box-start-bkgnd-hover.png
tab-left-hover.png / tab-left.png
tab-right-hover.png / tab-right.png

And for the record, tab-middle-hover.png and tab-middle.png are the same, too.

Comment 35

[Asaf Romano (gone)]

Attachment: less combined

well, if this helps... but not enough, I'll also combine the alltabs various images into this image (i didn't do that for the first pass since it requires some binding changes in tabbrowser-xml).

Comment 36

[Asaf Romano (gone)]

Attachment: corresponding image

Comment 37

[Asaf Romano (gone)]

Attachment: more...

Comment 38

[Asaf Romano (gone)]

Attachment: corresponding image

Comment 39

[Asaf Romano (gone)]

Attachment: don't regress the hover state

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 239921 [details] [diff] [review]
don't regress the hover state

Tested, works fine.

Comment 41

[Asaf Romano (gone)]

mozilla/toolkit/themes/winstripe/global/browser.css 1.21
mozilla/toolkit/themes/winstripe/global/globalBindings.xml 1.5
mozilla/toolkit/themes/winstripe/global/jar.mn 	1.24
mozilla/toolkit/themes/winstripe/global/icons/tabs.png 1.1

Comment 42

[Mike Connor [:mconnor]]

Comment on attachment 239921 [details] [diff] [review]
don't regress the hover state

let's get this in on branch while we look at the alltabs pieces

Comment 43

[Asaf Romano (gone)]

Comment on attachment 239921 [details] [diff] [review]
don't regress the hover state

Checked in on branch too.

Comment 44

[Asaf Romano (gone)]

OK :(

So, the patch here didn't really help Ts, not on branch at the very least.

However, after switching back to native-tabs, Ts on argo went down from 933ms to 907 ms.

Comment 45

[Mike Connor [:mconnor]]

This is basically the result of using transparent images instead of native theming.  Short of abandoning the new tabstrip long after the last possible minute, there's no realistic way of getting this back, so after a quick discussion with schrep, not going to block on this.

From a quick IRC conversation with vlad, the transparency is probably the real killer here, but cairo makes that hurt much less.  Will look at what we can do to help more on trunk, but for branch it seems like we'll just have to live with it.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> the transparency is probably the real killer here

That's possible, yeah.  Woudln't show so much in the profile if the slowness is happening over in X land, I guess....

Comment 47

[Asaf Romano (gone)]

Comment on attachment 239921 [details] [diff] [review]
don't regress the hover state

backed this out (branch & trunk)

Comment 48

[Dão Gottwald [:dao]]

(In reply to comment #45)
> but for branch it seems like we'll just have to live with it.

Well, as I said, the navigation toolbar icons don't necessarily have to be semi-transparent. I'd say it's too late to change them now, though ...

(In reply to comment #47)
> (From update of attachment 239921 [details] [diff] [review] [edit])
> backed this out (branch & trunk)

It was actually a good change anyway. It fixes that some images are two times in there. I.e. the first time you hover over a tab, the images take some time to load on slow / busy systems.

Comment 49

[Mike Connor [:mconnor]]

I don't think that we're going to move away from the transparent tab images, this is something that we're probably going to have to just live with until transparency doesn't murder us.

Comment 50

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #48)
> Well, as I said, the navigation toolbar icons don't necessarily have to be
> semi-transparent. I'd say it's too late to change them now, though ...

Do we want to change these now that we have the opportunity? Or is that more trouble than it's worth?

Clearing blocking as it's hard to see if this is still causing a huge Ts hit, but any thoughts on how to optimize performance of the theme would be great.

Comment 51

[Shawn Wilsher :sdwilsh]

I'm going through and marking old performance regression bugs as INCOMPLETE that are likely too old to be valid or get any traction on them.

Please re-open if you have more information or can demonstrate the regression still exists.