Closed
Bug 1293513
Opened 8 years ago
Closed 8 years ago
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)
Categories
(Toolkit :: Password Manager, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox48 | --- | unaffected |
firefox49 | --- | unaffected |
firefox50 | --- | unaffected |
firefox51 | --- | fixed |
People
(Reporter: ashiue, Assigned: MattN)
References
Details
(Keywords: perf, regression, talos-regression)
Attachments
(1 file)
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•8 years 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!
Updated•8 years ago
|
status-firefox48:
--- → unaffected
status-firefox49:
--- → unaffected
status-firefox50:
--- → unaffected
Assignee | ||
Comment 2•8 years ago
|
||
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.
Comment 3•8 years ago
|
||
Matt, thanks for looking into this, did you come up with any findings?
Assignee | ||
Comment 4•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Component: Untriaged → Password Manager
Product: Firefox → Toolkit
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
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) |
Assignee | ||
Comment 8•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Updated•8 years ago
|
Has Regression Range: --- → yes
Comment 9•8 years ago
|
||
that sounds like a good root cause- glad we have a path forward in the short and long term.
Comment 10•8 years 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•8 years 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
Comment 12•8 years ago
|
||
I already see a win on autoland for tp5o! https://treeherder.mozilla.org/perf.html#/alerts?id=2505
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/34909044f614
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 14•8 years ago
|
||
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.
Description
•