Closed
Bug 1388078
Opened 7 years ago
Closed 7 years ago
3.08% ts_paint_webext (windows7-32) regression on push 347ea0d06092d7fcd6c34e2c7ade389b0b71ae31 (Mon Aug 7 2017)
Categories
(Toolkit :: Form Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | fixed |
People
(Reporter: jmaher, Assigned: MattN)
References
Details
(Keywords: perf, regression, talos-regression, Whiteboard: [Form Autofill:M3])
Attachments
(1 file)
Talos has detected a Firefox performance regression from push: https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=347ea0d06092d7fcd6c34e2c7ade389b0b71ae31 As author of one of the patches included in that push, we need your help to address this regression. Regressions: 3% ts_paint_webext windows7-32 opt e10s 933.62 -> 962.42 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=8599 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 | ||
Updated•7 years ago
|
Component: Untriaged → Form Manager
Product: Firefox → Toolkit
Reporter | ||
Comment 1•7 years ago
|
||
this shows a slight regression in ts_paint as well, but it looks like the ts_paint with a web extension is what is flagged with alerts and is more visible. :mattn, I see you landed this a few hours ago, can you look into this alert to determine if there is something we can do to reduce the regression?
Flags: needinfo?(MattN+bmo)
Comment 2•7 years ago
|
||
This is apparently because addUpgradeListener winds up calling getAddonByID, which triggers an early DB load, which is quite expensive. Loading the add-on DB during startup is definitely unacceptable at this point, so this needs to be fixed.
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #2) > Loading the add-on DB during startup is definitely unacceptable at this > point, so this needs to be fixed. Do I assume correctly that this should be fixed in AOM?
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [Form Autofill:MVP]
Comment 4•7 years ago
|
||
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #3) > Do I assume correctly that this should be fixed in AOM? Yes, that would probably be the best option. But I don't have any time to spend on this at the moment, so that's probably a job for rhelmer or aswan.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 6•7 years ago
|
||
Note that bug 1387611 also needs to be uplifted to beta so getting towards a fix soon would be great. If it makes sense I can try dig into this a bit? From the duplicate bug 1388285 this also regressed: 948% tp5n nonmain_startup_fileio windows7-32 opt e10s 2,192.83 -> 22,975.92 8% sessionrestore windows10-64 opt e10s 658.92 -> 713.58 5% ts_paint windows10-64 opt e10s 717.83 -> 755.50 5% sessionrestore_no_auto_restore windows10-64 opt e10s 777.25 -> 814.53 5% ts_paint_webext linux64 pgo e10s 960.58 -> 1,004.83 4% sessionrestore windows7-32 opt e10s 747.25 -> 775.50 3% ts_paint osx-10-10 opt e10s 932.46 -> 964.25 3% ts_paint_webext linux64 opt e10s 1,117.46 -> 1,151.83
Assignee | ||
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(MattN+bmo)
Whiteboard: [Form Autofill:MVP] → [Form Autofill:M3]
Assignee | ||
Comment 7•7 years ago
|
||
Is there an observer notification I can listen to in the extension to know when the AOM DB is loaded at which time I could add the upgrade listener? Would a upgrade ever be queued before the DB is loaded? i.e. would that mean I would miss the chance to delay an extension update?
Flags: needinfo?(rhelmer)
Flags: needinfo?(aswan)
Assignee | ||
Comment 8•7 years ago
|
||
Maybe "EM-loaded", "xpi-database-loaded", or "addons-background-update-start" from a quick skim of AOM code: https://dxr.mozilla.org/mozilla-central/search?q=notifyObserver+path%3Amozapps%2Fextension+-path%3Atest&redirect=false
Comment 9•7 years ago
|
||
Waiting for "xpi-database-loaded" should be sufficient, yes.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
Thanks
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(rhelmer)
Flags: needinfo?(aswan)
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8896034 [details] Bug 1388078 - Form Autofill: Delay adding the AOM upgrade listener until the XPI DB is loaded. https://reviewboard.mozilla.org/r/167304/#review172532 ::: browser/extensions/formautofill/bootstrap.js:53 (Diff revision 2) > } > > if (data.hasOwnProperty("instanceID") && data.instanceID) { > + // Wait for the extension database to be loaded so we don't cause its init. > + Services.obs.addObserver(function xpiDatabaseLoaded() { > - AddonManager.addUpgradeListener(data.instanceID, (upgrade) => { > + AddonManager.addUpgradeListener(data.instanceID, (upgrade) => { Probably want to remove the observer here too.
Attachment #8896034 -
Flags: review?(rhelmer) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8896034 [details] Bug 1388078 - Form Autofill: Delay adding the AOM upgrade listener until the XPI DB is loaded. https://reviewboard.mozilla.org/r/167304/#review172596 sorry for the drive-by but you might also want to check `AddonManagerPrivate.isDBLoaded` first -- if that's true then you can just go ahead and add the listener right away but, moreover, if the database is already loaded then you're not going to see an xpi-database-loaded event. If everything is working as intended, it seems like this extension will only ever be started during browser startup when the database isn't yet loaded, but this would give you a little extra protection in case the database gets loaded really early or the extension is loaded later for some reason...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #14) > sorry for the drive-by but you might also want to check > `AddonManagerPrivate.isDBLoaded` first -- if that's true then you can just > go ahead and add the listener right away but, moreover, if the database is > already loaded then you're not going to see an xpi-database-loaded event. > If everything is working as intended, it seems like this extension will only > ever be started during browser startup when the database isn't yet loaded, > but this would give you a little extra protection in case the database gets > loaded really early or the extension is loaded later for some reason... Good point. I'll add that.
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/9ecc6b515324 Form Autofill: Delay adding the AOM upgrade listener until the XPI DB is loaded. r=rhelmer
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9ecc6b515324
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 20•7 years ago
|
||
Results look good: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=253a8560dc34456d2e8a13065e4b3eb5ecf6704f&newProject=mozilla-central&newRevision=f74094603063c92b710c0bae0bb5c092b915e92c&framework=1&filter=ts_paint&showOnlyImportant=0
Reporter | ||
Comment 21•7 years ago
|
||
I see some improvements from talos: == Change summary for alert #8713 (as of August 11 2017 05:22 UTC) == Improvements: 6% sessionrestore windows10-64 pgo e10s 489.75 -> 459.50 5% ts_paint windows10-64 pgo e10s 629.00 -> 595.00 5% ts_paint_webext windows7-32 pgo e10s 780.25 -> 740.67 5% sessionrestore linux64 pgo e10s 473.71 -> 449.75 5% ts_paint_webext windows10-64 opt e10s 825.54 -> 787.58 5% sessionrestore windows7-32 opt e10s 757.83 -> 723.67 4% ts_paint_webext windows10-64 pgo e10s 687.50 -> 656.69 4% ts_paint windows7-32 opt e10s 850.62 -> 814.17 4% ts_paint windows10-64 opt e10s 752.25 -> 722.42 4% ts_paint_webext windows7-32 opt e10s 946.50 -> 912.75 3% sessionrestore osx-10-10 opt e10s 727.75 -> 702.33 3% ts_paint linux64 pgo e10s 822.05 -> 793.92 3% sessionrestore_no_auto_restore windows7-32 opt e10s851.70 -> 823.08 3% ts_paint linux64 opt e10s 930.50 -> 899.50 2% ts_paint_webext osx-10-10 opt e10s 1,026.54 -> 1,001.58 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8713
Comment 22•7 years ago
|
||
Hi MattN, Since this is a regression, do you think this is worth uplifting to Beta56?
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 23•7 years ago
|
||
This isn't on 56 yet as autofill isn't enabled there yet. The fix will be included with the uplift of the system add-on.
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 24•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f6117c7b1161
You need to log in
before you can comment on or make changes to this bug.
Description
•