12.41% tabpaint (windows8-64) regression on push 5323eee5a384afe38f3bd235a58e1713914ef169 (Wed Apr 19 2017)

RESOLVED FIXED

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: igoldan, Unassigned)

Tracking

({perf, regression, talos-regression})

unspecified
Unspecified
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [qf-])

Talos has detected a Firefox performance regression from push 5323eee5a384afe38f3bd235a58e1713914ef169. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 12%  tabpaint summary windows8-64 opt      84.16 -> 94.61


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=6100

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling
Afer an overview on the changesets included in this patch, looks like bug 1352069 is more related to this issue. Could you all take a look at your changes or confirm this bug, so to narrow down on the exact regression origin.
Flags: needinfo?(ttromey)
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(snorp)
Flags: needinfo?(kchen)
Flags: needinfo?(continuation)
the windows 8 tests take so long to run (large backlog) that we are waiting for more data before knowing which patch is the root cause, any help we can get ahead of time will speed this process up!
The retriggers show that bug 1352069 is indeed the related one.
:squib could you take a look at this regression?
Flags: needinfo?(ttromey)
Flags: needinfo?(snorp)
Flags: needinfo?(kchen)
Flags: needinfo?(continuation)
I don't see anything in here that could affect performance. (Although I do see one small bug in the migration code that slipped through.)
Flags: needinfo?(squibblyflabbetydoo)
:squib, is there a fix for this or can you help explain this and recommend leaving the regression in?  This is a 12% regression on windows 8, so I would like to understand why we would be accepting this?
Flags: needinfo?(squibblyflabbetydoo)
Having looked at this some more, I may have figured out the problem, but I'll need to ask some folks what the right way to fix it is.

`testing/talos/talos/tests/tabpaint/` has a `tabpaint-signed.xpi` file that appears to be an archive of all the other files in the directory. Notably, this includes `bootstrap.js`, which I patched in bug 1352069 to use the new pref name. However, I didn't update `tabpaint-signed.xpi`, so if that's where the *real* code lives, the test would be wrong. That said, I don't know anything about why that XPI lives there or what the right way to update it is. (I also don't know why we'd only see a regression on Win 8...)
Flags: needinfo?(squibblyflabbetydoo)
oh, since we run tests with signed addons, we need to sign the addon, that is why there is a .xpi file in the directory.  Some instructions for signing this are here:
https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions#Use_Temporary_Addons


I don't mind signing this and getting the new .xpi into a patch, let me know if that is preferred.
I'll make the patch; it'd be a good opportunity to learn how to do this correctly in case I ever need to in the future.

In the longer-term, maybe it makes sense to *enable* animations for the Talos test? Currently, the test doesn't really reflect what happens on end users' machines...
I think enabling animations would be a reasonable request- is this done with a preference?  We would need to determine which tests this should be enabled for (all?) and do some try pushes to see the impact.  As long as the tests are not less reliable (noisy) I would have no problem with this.  Getting closer to end users is always good!
Yeah, the preference is `toolkit.cosmeticAnimations.enabled`; it used to be `browser.tabs.animate` (plus a few others). The issue here is that the tabpaint test disabled tab animations, but when we migrated to the new pref, I didn't realize I needed to update the XPI in the tabpaint folder. As a result, our tests went from checking without animations to checking *with*.

I'll file a follow-up to enable animations in our performance tests. I think it makes more sense to disable them for now in this bug so that we can ensure that there's not something else causing a performance regression.
Component: Untriaged → Preferences
Component: Preferences → Talos
Product: Firefox → Testing
On second thought, I keep getting sidetracked by other important work and haven't made much progress in getting everything set up to fix this bug. Do you mind building the XPI, :jmaher? Sorry for the wishy-washiness!
Flags: needinfo?(jmaher)
We should track this significant tabpaint regression on windows8-64 for Quantum Flow.
Whiteboard: [qf]
We should definitely fix this, but per comment 6, this is* just an issue with the tests. It won't affect end users.

* Well, I *think* it is...
Whoops, hit Submit too fast.

It's a little concerning that this is only a significant regression on win8-64, which might imply that our tab animations are janky on that platform. However, I think we'd need to investigate a bit more to see if that's actually the case...
Whiteboard: [qf] → [qf-]

Comment 15

2 years ago
(In reply to Jim Porter (:squib) from comment #4)
> I don't see anything in here that could affect performance. (Although I do
> see one small bug in the migration code that slipped through.)

FWIW overreading prefs certainly affects performance negatively.
Depends on: 1365289
ok, I am handling this in bug 1365289
Flags: needinfo?(jmaher)
Joel, now that the tabpaint addon is signed (bug 1365289), is there anything that still needs to be done for this bug? win8's measurements appear to have improved after May 24 right before we switched to win10:

https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-central,a9cd333dff68ce0812dc85e0657af4edfc51ebe3,1,1%5D&series=%5Bmozilla-central,0bec96d78bc54370bd027af09bdd0edc8df7afd7,1,1%5D&series=%5Bmozilla-central,26721ba0e181e2844da3ddc2284a331ba54eefe0,1,1%5D&timerange=7776000
Flags: needinfo?(jmaher)
this is interesting as we turned off non-e10s tests on May 16th and this is a regression only on non-e10s.  The problem is we landed the fix for the signed addon on May 17th- so there is no way to verify this.  We can assume the signed addon would solve the problem based on the fact that we were missing a preference.

I would recommend closing this bug as fixed.
Flags: needinfo?(jmaher)
Resolving as fixed, then. Thanks!
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.