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)

53 Branch
defect
Not set
normal

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
Component: Untriaged → Form Manager
Product: Firefox → Toolkit
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)
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.
(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]
(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)
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
Flags: needinfo?(MattN+bmo)
Whiteboard: [Form Autofill:MVP] → [Form Autofill:M3]
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)
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
Waiting for "xpi-database-loaded" should be sufficient, yes.
Thanks
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(rhelmer)
Flags: needinfo?(aswan)
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 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...
(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.
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
https://hg.mozilla.org/mozilla-central/rev/9ecc6b515324
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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
Hi MattN,
Since this is a regression, do you think this is worth uplifting to Beta56?
Flags: needinfo?(MattN+bmo)
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)
You need to log in before you can comment on or make changes to this bug.