2.45 - 12.92% tart / tp5o / tp5o responsiveness (linux64, osx-10-10, windows7-32, windows8-64, windowsxp) regression on push f54fea6078ddf0023a4307a25264c6ece653e503 (Sat Aug 6 2016)

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Password Manager
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Alison Shiue, Assigned: MattN)

Tracking

({perf, regression, talos-regression})

51 Branch
mozilla51
perf, regression, talos-regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox50 unaffected, firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

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

Summary of tests that regressed:

  tp5o summary windows7-32 opt: 363.9 -> 378.27 (3.95% worse)
  tp5o responsiveness windows7-32 opt: 80.28 -> 90.66 (12.92% worse)
  tp5o summary windows8-64 opt: 325.32 -> 338.89 (4.17% worse)
  tp5o responsiveness linux64 opt: 48.5 -> 53.42 (10.15% worse)
  tp5o summary linux64 opt e10s: 341.66 -> 350.04 (2.45% worse)
  tp5o summary osx-10-10 opt: 286.69 -> 295.65 (3.13% worse)
  tp5o summary windows7-32 opt e10s: 349.82 -> 359.84 (2.86% worse)
  tp5o summary windows8-64 opt e10s: 303.17 -> 312.13 (2.95% worse)
  tp5o summary windows7-32 pgo e10s: 254.85 -> 262.79 (3.11% worse)
  tp5o summary windows7-32 pgo: 259.77 -> 270.71 (4.21% worse)
  tp5o summary linux64 opt: 340.23 -> 350.88 (3.13% worse)
  tp5o summary windowsxp opt: 363.6 -> 384.14 (5.65% worse)
  tart summary windows7-32 opt e10s: 6.46 -> 6.66 (2.95% worse)
  tp5o summary windowsxp opt e10s: 348.68 -> 362.33 (3.91% worse)


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

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
(Reporter)

Comment 1

a year ago
This issue might be caused by push:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f8fc21bb232095b7b4c995d259558212cb90a44b&tochange=f54fea6078ddf0023a4307a25264c6ece653e503

Hi Matthew, as you are the patch author, can you take a look at this and determine what is the root cause? Thanks!
Flags: needinfo?(MattN+bmo)
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
I suspect this is related to the progress listener in bug 1166947 but I'm surprised that it would affect TART as I don't think there are <input type=password>. The progress listener should only be setup on pages with password fields and should be removed otherwise. I'll investigate this more tomorrow.
Matt, thanks for looking into this, did you come up with any findings?
I haven't had too much time to look into this yet but started on Thursday (I was on PTO Friday). I started by trying to see if the pages that reduced responsiveness had password fields on them:
> sed 's/^http:\/\/localhost\/page_load_test\/tp5n\///' tp5o.manifest | sed 's/\/.*$//' | xargs -n 1 grep -E -R -i -l --include "*.htm*" 'type=.?password' {} \; | sed 's/\/.*//'

The following contain /type=.?password/:
163.com
56.com
beatonna.livejournal.com
digg.com
ezinearticles.com
homeway.com.cn
ifeng.com
indiatimes.com
mail.ru
myspace.com
naver.com
reddit.com
uol.com.br
wsj.com
yelp.com

I used the page set from http://people.mozilla.org/~jmaher/taloszips/zips/tp5n.zip which I hope is up-to-date.

If I look at https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=fx-team&originalRevision=6a5f939e29dec24277cc20696887e7c2eb1e2358&newProject=fx-team&newRevision=f54fea6078ddf0023a4307a25264c6ece653e503&originalSignature=764594bc71db36a91df2e91a960a3a55b4fe456a&newSignature=764594bc71db36a91df2e91a960a3a55b4fe456a&showOnlyImportant=1&showOnlyConfident=1&framework=1 then the following regressed significantly:
* 163.com
* chinaz.com
* cnn.com
* imdb.com
* mashable.com

The only overlap between the two sets is 163.com which is discouraging since bug 1166947 shouldn't affect pages that don't have <input type="password">. Is it possible that 163.com is the only one that regressed and the others are noise?
No longer blocks: 1291346
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Component: Untriaged → Password Manager
Product: Firefox → Toolkit
Comment hidden (mozreview-request)
I noticed that we are setting up the progress listener in onDOMInputPasswordAdded even when the input is in a form though bug 1166947 was only aiming to help with formless inputs. Handling password inputs in a form but without a submit event was split into bug 1287202 so this patch may just delay the problem until bug 1287202 but gets rid of the extra work in the meantime.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a85791d90c6
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
PerfHerder results are looking good so far when comparing to the push before bug 1166947 landed:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=f8fc21bb2320&newProject=try&newRevision=8a85791d90c6442a9a51649fc92af038773e7cff&framework=1
Priority: -- → P1
Has Regression Range: --- → yes
that sounds like a good root cause- glad we have a path forward in the short and long term.

Comment 10

a year ago
mozreview-review
Comment on attachment 8781298 [details]
Bug 1293513 - Only setup login manager's progress listener for formless passwords inputs.

https://reviewboard.mozilla.org/r/71722/#review69534
Attachment #8781298 - Flags: review?(dolske) → review+

Comment 11

a year ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/34909044f614
Only setup login manager's progress listener for formless passwords inputs. r=Dolske
I already see a win on autoland for tp5o!

https://treeherder.mozilla.org/perf.html#/alerts?id=2505

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34909044f614
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
looking at this, I see that we fixed the majority of the regression, I am happy with that.
You need to log in before you can comment on or make changes to this bug.