Bug 1456262: Optimize addAddonsToCrashReporter at startup. r?aswan draft
authorKris Maglione <maglione.k@gmail.com>
Sun, 22 Apr 2018 15:58:02 -0700
changeset 786809 287c78167a70745595eaa57c6fcd9712ad464b9e
parent 786808 19001aac9f3e279b287509b4ceff924b82f6dc96
child 786821 85d0355fe00be803e8c013c7e2979534a86ac518
push id107577
push usermaglione.k@gmail.com
push dateMon, 23 Apr 2018 21:40:28 +0000
reviewersaswan
bugs1456262
milestone61.0a1
Bug 1456262: Optimize addAddonsToCrashReporter at startup. r?aswan This consistently shows up in profiles as taking about 6-10ms during startup. Part of the problem is that it loads TelemetrySession.jsm, which we can't fix. Part of the problem is that the escaping and string manipulation it does is fairly expensive. We can cache the escaped values in addonStartup.jsm to avoid that. A lot of the problem is that we call it every time we load a bootstrap scope, even though we already set the correct value before we start loading any bootstrapped scopes, and it doesn't change after that. We can avoid this by skipping the redundant calls during startup. MozReview-Commit-ID: 68EWdnJdPvk
toolkit/mozapps/extensions/internal/XPIProvider.jsm
--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -22,16 +22,17 @@ XPCOMUtils.defineLazyModuleGetters(this,
   Langpack: "resource://gre/modules/Extension.jsm",
   LightweightThemeManager: "resource://gre/modules/LightweightThemeManager.jsm",
   FileUtils: "resource://gre/modules/FileUtils.jsm",
   PermissionsUtils: "resource://gre/modules/PermissionsUtils.jsm",
   OS: "resource://gre/modules/osfile.jsm",
   ConsoleAPI: "resource://gre/modules/Console.jsm",
   JSONFile: "resource://gre/modules/JSONFile.jsm",
   LegacyExtensionsUtils: "resource://gre/modules/LegacyExtensionsUtils.jsm",
+  TelemetrySession: "resource://gre/modules/TelemetrySession.jsm",
 
   XPIDatabase: "resource://gre/modules/addons/XPIDatabase.jsm",
   XPIDatabaseReconcile: "resource://gre/modules/addons/XPIDatabase.jsm",
   XPIInstall: "resource://gre/modules/addons/XPIInstall.jsm",
   verifyBundleSignedState: "resource://gre/modules/addons/XPIInstall.jsm",
 });
 
 XPCOMUtils.defineLazyServiceGetter(this, "aomStartup",
@@ -517,16 +518,17 @@ const JSON_FIELDS = Object.freeze([
   "dependencies",
   "enabled",
   "file",
   "hasEmbeddedWebExtension",
   "lastModifiedTime",
   "path",
   "runInSafeMode",
   "startupData",
+  "telemetryKey",
   "type",
   "version",
 ]);
 
 const BOOTSTRAPPED_FIELDS = Object.freeze([
   "dependencies",
   "hasEmbeddedWebExtension",
   "runInSafeMode",
@@ -544,16 +546,20 @@ class XPIState {
     this.bootstrapped = false;
 
     for (let prop of JSON_FIELDS) {
       if (prop in saved) {
         this[prop] = saved[prop];
       }
     }
 
+    if (!this.telemetryKey) {
+      this.telemetryKey = this.getTelemetryKey();
+    }
+
     if (saved.currentModifiedTime && saved.currentModifiedTime != this.lastModifiedTime) {
       this.lastModifiedTime = saved.currentModifiedTime;
       this.changed = true;
     }
   }
 
   /**
    * Migrates an add-on's data from xpiState and bootstrappedAddons
@@ -636,16 +642,17 @@ class XPIState {
    * @returns {Object}
    */
   toJSON() {
     let json = {
       enabled: this.enabled,
       lastModifiedTime: this.lastModifiedTime,
       path: this.relativePath,
       version: this.version,
+      telemetryKey: this.telemetryKey,
     };
     if (this.type != "extension") {
       json.type = this.type;
     }
     if (this.bootstrapped) {
       json.bootstrapped = true;
       json.dependencies = this.dependencies;
       json.runInSafeMode = this.runInSafeMode;
@@ -678,16 +685,26 @@ class XPIState {
     }
 
     this.changed = mtime != this.lastModifiedTime;
     this.lastModifiedTime = mtime;
     return this.changed;
   }
 
   /**
+   * Returns a string key by which to identify this add-on in telemetry
+   * and crash reports.
+   *
+   * @returns {string}
+   */
+  getTelemetryKey() {
+    return encoded`${this.id}:${this.version}`;
+  }
+
+  /**
    * Update the XPIState to match an XPIDatabase entry; if 'enabled' is changed to true,
    * update the last-modified time. This should probably be made async, but for now we
    * don't want to maintain parallel sync and async versions of the scan.
    *
    * Caller is responsible for doing XPIStates.save() if necessary.
    *
    * @param {DBAddonInternal} aDBAddon
    *        The DBAddonInternal for this add-on.
@@ -705,16 +722,18 @@ class XPIState {
     this.enabled = aDBAddon.visible && !aDBAddon.disabled;
 
     this.version = aDBAddon.version;
     this.type = aDBAddon.type;
     if (aDBAddon.startupData) {
       this.startupData = aDBAddon.startupData;
     }
 
+    this.telemetryKey = this.getTelemetryKey();
+
     this.bootstrapped = !!aDBAddon.bootstrap;
     if (this.bootstrapped) {
       this.hasEmbeddedWebExtension = aDBAddon.hasEmbeddedWebExtension;
       this.dependencies = aDBAddon.dependencies;
       this.runInSafeMode = canRunInSafeMode(aDBAddon);
     }
 
     if (aUpdated || mustGetMod) {
@@ -1849,25 +1868,22 @@ var XPIProvider = {
     }
 
     // In safe mode no add-ons are loaded so we should not include them in the
     // crash report
     if (Services.appinfo.inSafeMode) {
       return;
     }
 
-    let data = Array.from(XPIStates.enabledAddons(),
-                          a => encoded`${a.id}:${a.version}`).join(",");
+    let data = Array.from(XPIStates.enabledAddons(), a => a.telemetryKey).join(",");
 
     try {
       Services.appinfo.annotateCrashReport("Add-ons", data);
     } catch (e) { }
 
-    let TelemetrySession =
-      ChromeUtils.import("resource://gre/modules/TelemetrySession.jsm", {}).TelemetrySession;
     TelemetrySession.setAddOns(data);
   },
 
   /**
    * Check the staging directories of install locations for any add-ons to be
    * installed or add-ons to be uninstalled.
    *
    * @param {Object} aManifests
@@ -2519,28 +2535,35 @@ var XPIProvider = {
    * @param {string} aType
    *        The type for the add-on
    * @param {boolean} aRunInSafeMode
    *        Boolean indicating whether the add-on can run in safe mode.
    * @param {string[]} aDependencies
    *        An array of add-on IDs on which this add-on depends.
    * @param {boolean} hasEmbeddedWebExtension
    *        Boolean indicating whether the add-on has an embedded webextension.
+   * @param {integer?} [aReason]
+   *        The reason this bootstrap is being loaded, as passed to a
+   *        bootstrap method.
    */
   loadBootstrapScope(aId, aFile, aVersion, aType, aRunInSafeMode, aDependencies,
-                     hasEmbeddedWebExtension) {
+                     hasEmbeddedWebExtension, aReason) {
     this.activeAddons.set(aId, {
       bootstrapScope: null,
       // a Symbol passed to this add-on, which it can use to identify itself
       instanceID: Symbol(aId),
       started: false,
     });
 
-    // Mark the add-on as active for the crash reporter before loading
-    this.addAddonsToCrashReporter();
+    // Mark the add-on as active for the crash reporter before loading.
+    // But not at app startup, since we'll already have added all of our
+    // annotations before starting any loads.
+    if (aReason !== BOOTSTRAP_REASONS.APP_STARTUP) {
+      this.addAddonsToCrashReporter();
+    }
 
     let activeAddon = this.activeAddons.get(aId);
 
     logger.debug("Loading bootstrap scope from " + aFile.path);
 
     let principal = Cc["@mozilla.org/systemprincipal;1"].
                     createInstance(Ci.nsIPrincipal);
 
@@ -2642,17 +2665,18 @@ var XPIProvider = {
     }
 
     try {
       // Load the scope if it hasn't already been loaded
       let activeAddon = this.activeAddons.get(aAddon.id);
       if (!activeAddon) {
         this.loadBootstrapScope(aAddon.id, aFile, aAddon.version, aAddon.type,
                                 runInSafeMode, aAddon.dependencies,
-                                aAddon.hasEmbeddedWebExtension || false);
+                                aAddon.hasEmbeddedWebExtension || false,
+                                aReason);
         activeAddon = this.activeAddons.get(aAddon.id);
       }
 
       if (aMethod == "startup" || aMethod == "shutdown") {
         if (!aExtraParams) {
           aExtraParams = {};
         }
         aExtraParams.instanceID = this.activeAddons.get(aAddon.id).instanceID;