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)

51 Branch
defect

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
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)
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
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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/34909044f614
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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.