4% TART regression on osx 10.8 found on fx-team from revision 1adbd1657ba1

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jmaher, Assigned: dao)

Tracking

({perf, regression})

Trunk
Firefox 31
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [talos_regression])

Attachments

(2 attachments, 1 obsolete attachment)

Talos sent an alert and here is the graph for a regression:
http://graphs.mozilla.org/graph.html#tests=[[293,64,24]]&sel=1397423501000,1397596301000&displayrange=7&datatype=running

This only seems to affect osx 10.8.

I did some retriggers to verify this was the real push of concern:
https://tbpl.mozilla.org/?tree=Fx-Team&fromchange=b8081c65fd70&tochange=12796fe3eb01&jobname=Rev5%20MacOSX%20Mountain%20Lion%2010.8%20fx-team%20talos%20svgr


here is a link to datazilla:
https://datazilla.mozilla.org/?start=1397073863&stop=1397678663&product=Firefox&repository=Mozilla-Inbound&os=mac&os_version=OS%20X%2010.8&test=tart&project=talos

and these are the subtests which are regressed:
simple-close-DPI1.all.TART
icon-close-DPI2.all.TART
icon-close-DPI1.all.TART
Dao, can you take a look at this patch and weigh in on what might be the problem?
Flags: needinfo?(dao)
Assignee

Comment 2

5 years ago
(In reply to Joel Maher (:jmaher) from comment #0)
> and these are the subtests which are regressed:
> simple-close-DPI1.all.TART
> icon-close-DPI2.all.TART
> icon-close-DPI1.all.TART

It's not clear to me what these subtests are doing...
Flags: needinfo?(avihpit)
(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to Joel Maher (:jmaher) from comment #0)
> > and these are the subtests which are regressed:
> > simple-close-DPI1.all.TART
> > icon-close-DPI2.all.TART
> > icon-close-DPI1.all.TART
> 
> It's not clear to me what these subtests are doing...

I can answer that one.

These subtests measure the average time it takes to paint the frames of the tab close animation when:

1) Closing a tab without a favicon at DPI1 (simple-close-DPI1)
2) Closing a tab with a favicon at DPI2 (icon-close-DPI2)
3) Closing a tab with a favicon at DPI1 (icon-close-DPI1).

Because we're seeing a regression in the .all measures, but not the .half, it indicates that the expensive frames are in the first half of the animation.

So something about the first frames of the close animation is slower to paint.
Assignee

Comment 4

5 years ago
Ok, I have an idea what's going on.
Assignee: nobody → dao
Flags: needinfo?(dao)
Flags: needinfo?(avihpit)
Assignee

Comment 5

5 years ago
Posted patch patch (obsolete) — Splinter Review
So basically we were animating the label and close button for closing tabs even though they weren't rendered, because closing tabs have visibility:hidden.

10.8 has an enormous test queue. No idea when I will get results there. However, I see slightly improving tart numbers on all other platforms:

https://tbpl.mozilla.org/?tree=Try&rev=b400f44887ed
https://tbpl.mozilla.org/?tree=Try&rev=70e3a56e33dd
http://compare-talos.mattn.ca/?oldRevs=b400f44887ed&newRev=70e3a56e33dd&server=graphs.mozilla.org&submit=true
Attachment #8408158 - Flags: review?(jaws)
:dao, thanks for taking a look at this.  Every little fix makes a difference!
Comment on attachment 8408158 [details] [diff] [review]
patch

Review of attachment 8408158 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.css
@@ +132,5 @@
>  
>  .tab-background:not([fadein]):not([pinned]) {
>    visibility: hidden;
> +  /* Closing tabs are completely hidden without a delay. */
> +  transition: none;

Why don't we just change the rule to the following:

.tab-background[fadein] {
  transition: visibility 0ms 25ms;
}

.tab-close-button[fadein],
.tab-label[fadein] {
  transition: opacity 70ms 230ms,
              visibility 0ms 230ms;
}
Assignee

Comment 8

5 years ago
Posted patch patch v2Splinter Review
Attachment #8408158 - Attachment is obsolete: true
Attachment #8408158 - Flags: review?(jaws)
Attachment #8408885 - Flags: review?(jaws)
According to the compare-talos, this approach will not gain us back the 4% that we've lost here...
Comment on attachment 8408885 [details] [diff] [review]
patch v2

Review of attachment 8408885 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!
Attachment #8408885 - Flags: review?(jaws) → review+
:dao and :jaws, thanks for the patch and review.  I know this isn't a 100% fix according to comment 9, but bringing us closer to fixing this is great to see.
Assignee

Updated

5 years ago
Keywords: leave-open
looking at graph server, I don't see a big win here:
http://graphs.mozilla.org/graph.html#tests=[[293,64,24]]&sel=none&displayrange=7&datatype=running

prior to the regression: ~11.0
after the patch landed:  ~11.5
after this fix landed:   ~11.4

It appears to be reduced slightly.
Assignee

Comment 15

5 years ago
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=f7320bbfb006&newRev=bde5e60ad7e6&submit=true

I don't understand why this doesn't undo the regression. I'm starting to think that the tart test is measuring something wrong :/
Does "visibility: hidden" instead of display:none undo the regression?
Assignee

Comment 17

5 years ago
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #16)
> Does "visibility: hidden" instead of display:none undo the regression?

visibility:hidden on tab-stack should be redundant since the whole tab already has that while closing.
Assignee

Comment 18

5 years ago
Bug 995626 has been backed out.
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.