980433 - Multiple talos regressions after landing bug 870593

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike de Boer [:mikedeboer]]

Relevant tree-management thread:

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/c8Tf1fFIrdc

Summary of regressions:

Fx-Team-Non-PGO - Paint - Ubuntu HW 12.04 x64 - 20.9% increase
Fx-Team-Non-PGO - Ts, Paint - Ubuntu HW 12.04 x64 - 5.17% increase
Fx-Team-Non-PGO - Tp5 Optimized - Ubuntu HW 12.04 x64 - 2.26% increase
Fx-Team-Non-PGO - Ts, Paint - Ubuntu HW 12.04 - 5.57% increase
Fx-Team-Non-PGO - tscroll-ASAP - Ubuntu HW 12.04 x64 - 4.6% increase
Fx-Team-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 x64 - 54.1% increase
Fx-Team-Non-PGO - SVG-ASAP - Ubuntu HW 12.04 x64 - 13.1% increase

Comment 1

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #0)
> Fx-Team-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 x64 - 54.1% increase


FWIW, I think Matt and I aren't convinced that's just bug 870593's fault... could be one of the other things in that range as well.

Comment 2

[Mike de Boer [:mikedeboer]]

Try push of a fix, local produce: https://tbpl.mozilla.org/?tree=Try&rev=7078ef2f254a

Comment 3

[Mike de Boer [:mikedeboer]]

Fx-Team-Non-PGO - Paint - WINNT 6.2 x64 - 2.56%

Comment 4

[Mike de Boer [:mikedeboer]]

Fx-Team-Non-PGO - tscroll-ASAP - Ubuntu HW 12.04 - 2.43% increase
Fx-Team-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 - 41.2% increase
Fx-Team-Non-PGO - SVG-ASAP - Ubuntu HW 12.04 - 9.61% increase

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: hide the tip panel by default to prevent Talso regressions

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8387063 [details] [diff] [review]
Patch v1: hide the tip panel by default to prevent Talso regressions

http://compare-talos.mattn.ca/?oldRevs=2f1e13cd6e16&newRev=7078ef2f254a&server=graphs.mozilla.org&submit=true

Comment 7

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

Comment on attachment 8387063 [details] [diff] [review]
Patch v1: hide the tip panel by default to prevent Talso regressions

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

Looking at the compare talos, the only thing that immediately stood out was the 12% ts_paint regression.

_HOWEVER_

looking at the individual ts_paint numbers in your "after" push[1], it looks like you have a few outliers in the 1000's, which looking at the history of ts_paint, is not unheard of. So I think that's all we're seeing here - some unlucky outliers.

Otherwise, it's clearly putting as back in the black. Nice one, Mike!

[1]: https://tbpl.mozilla.org/?tree=Try&rev=7078ef2f254a

r=me with my nit fixed.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +568,5 @@
>          this.hideTip();
>        });
>      }
>  
> +    this.tipPanel.setAttribute("hidden", false);

Let's do this.tipPanel.removeAttribute("hidden") instead.

Comment 8

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

(In reply to Mike Conley (:mconley) from comment #7)
> > +    this.tipPanel.setAttribute("hidden", false);
> 
> Let's do this.tipPanel.removeAttribute("hidden") instead.

Better yet, this.tipPanel.hidden = false;

(hidden is a "magic" XUL attribute: http://hg.mozilla.org/mozilla-central/annotate/02506bdd5bd8/content/xul/content/src/nsXULElement.cpp#l1415)

Comment 9

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

Sounds good to me.

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: hide the tip panel by default to prevent Talso regressions

w/ nits fixed. Carrying over r=mconley.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/1466debaadb4

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1466debaadb4

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8387104 [details] [diff] [review]
Patch v1.1: hide the tip panel by default to prevent Talso regressions

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 870593
User impact if declined: none, this is mainly to fight the Talos perf regression
Testing completed (on m-c, etc.): merged to m-c
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a

Comment 14

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

An addendum - user impact if declined is that the user is likely to experience slower times for the window to paint, and jankier tabs. Basically, this patch just makes everything faster and better for our users.

Comment 15

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

Comment on attachment 8387104 [details] [diff] [review]
Patch v1.1: hide the tip panel by default to prevent Talso regressions

I would have just rolled this into the Aurora patch for bug 870593 if I were you, but it doesn't hurt to track them separately.

Comment 16

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

(we certainly can't land that patch without this one, though)

Comment 17

[Mike de Boer [:mikedeboer]]

Landed in one combined patch on aurora, together with bug 870593 and bug 980369:

https://hg.mozilla.org/releases/mozilla-aurora/rev/92c7529e9169

Comment 18

[Mike de Boer [:mikedeboer]]

backed-out (faulty commit msg): https://hg.mozilla.org/releases/mozilla-aurora/rev/2c495062cd5c
relanded: https://hg.mozilla.org/releases/mozilla-aurora/rev/a97cdf6cdf40