Closed Bug 997382 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: jmaher, Assigned: dao)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Attachments

(2 files, 1 obsolete file)

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)
(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.
Ok, I have an idea what's going on.
Assignee: nobody → dao
Flags: needinfo?(dao)
Flags: needinfo?(avihpit)
Attached 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;
}
Attached 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.
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.
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?
(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.
Bug 995626 has been backed out.
Status: NEW → RESOLVED
Closed: 10 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.