Extension manager is flush happy

RESOLVED FIXED in mozilla1.9.1a2

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({fixed1.9.0.2, perf})

Trunk
mozilla1.9.1a2
fixed1.9.0.2, perf
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
Following on from bug 396695 comment 44 we seem to be flushing the extensions datasource an awful low, often on startup. I'm going to work on some fixes to reduce the number of flushes while keeping things safe and I also have a plan that may slightly improve the responsiveness of the UI.

A few metrics with things the way they are now:

Installing a new extension (before restart), 13 flushes. During the restart another 4.
Installing an upgrade to an extension (before restart), 13 flushes. During the restart another 14.
Performing an update check for an extension, 3 or 4 if an update is found (that is per extension).

Disturbingly running a new build of firefox (version number didn't change) in a profile that last had an old build run (it detected the 2 app addons as changed but none others), 45 flushes.
(Assignee)

Comment 1

11 years ago
(In reply to comment #0)
> Disturbingly running a new build of firefox (version number didn't change) in a
> profile that last had an old build run (it detected the 2 app addons as changed
> but none others), 45 flushes.

This is 18 flushes per detected changed addon (7 in _upgradeItem, 10 in _configureForthcomingItem, 1 in _addItemMetadata) followed by 9 flushes setting appDisabled to null (which it already was in the first place) on every installed addon.
(Assignee)

Comment 2

11 years ago
Created attachment 286693 [details] [diff] [review]
rough first cut

I haven't properly checked over this so not requesting review just yet but I don't know what you think of this as a plan Rob?

Basically it changes a number of calls to setItemProperty ro just _setProperty with a single Flush, then for common operations that really should flush at the end like _configureForthcomingItem, _upgradeItem and the mail startup paths surround everything in an updatebatch, then put locks in the datasource so it doesn't flush during an updatebatch, just remembers that one has been requested and does it at the end of the batch.

This currently drops the number of flushes on startup to generally 1 if any in-app changes have been made, or 2 if file changes are detected. An install before restart is 4 flushes
(Assignee)

Comment 3

10 years ago
Looking at actual metrics from dtrace this is only incurring a very minor hit in comparison with the rest of the time spent in the EM during startup so I don't believe it is worth fixing in this way.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 4

10 years ago
Fixing this would greatly alleviate bug 431065
Blocks: 431065
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → NEW
(Assignee)

Comment 5

10 years ago
Created attachment 329403 [details] [diff] [review]
patch rev 1

This adds an explicit block to flushing during the startup sequence, only performing the flushes at the end if necessary. It also adds a setItemProperties method that can be used to set multiple properties with only a single flush at the end and is used in preference to a flush per property in various places.
Attachment #286693 - Attachment is obsolete: true
Attachment #329403 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Dave, not sure but does it make sense to make sure everything has been flushed in _shutdown() just in case?
(Assignee)

Comment 7

10 years ago
Created attachment 330926 [details] [diff] [review]
patch rev 2

Certainly no harm in it.
Attachment #329403 - Attachment is obsolete: true
Attachment #330926 - Flags: review?(robert.bugzilla)
Attachment #329403 - Flags: review?(robert.bugzilla)
Comment on attachment 330926 [details] [diff] [review]
patch rev 2

This looks good... just a few nits

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>@@ -172,16 +172,20 @@ var gConsole              = null;
> var gConsole              = null;
> var gInstallManifestRoot  = null;
> var gVersionChecker       = null;
> var gLoggingEnabled       = null;
> var gCheckCompatibility   = true;
> var gCheckUpdateSecurity  = true;
> var gLocale               = "en-US";
> var gFirstRun             = false;
>+
nit: the vars aren't grouped by use currently but I could go either way with this.

>+var gAllowFlush           = true;
>+var gDSNeedsFlush         = false;
>+var gManifestNeedsFlush   = false;

>@@ -2656,16 +2660,34 @@ ExtensionManager.prototype = {
>     param.AppendElement(arg);
>     ww.openWindow(null, EMURL, null, EMFEATURES, param);
>   },
> 
>   /**
>    * Clean up on application shutdown to avoid leaks.
>    */
>   _shutdown: function() {
>+    if (!gAllowFlush) {
>+      // Something went wrong and there are potentially flushes pending.
>+      try {
>+        gAllowFlush = true;
>+        if (gManifestNeedsFlush) {
>+          gManifestNeedsFlush = false;
>+          this._updateManifests(false);
>+        }
>+        if (gDSNeedsFlush) {
>+          gDSNeedsFlush = false;
>+          this.datasource.Flush();
>+        }
>+      }
>+      catch (e) {
>+        ERROR("Error flushing caches: " + e);
>+      }
>+    }
Since we shouldn't get into this state in the first place it might be helpful to add a call to ERROR at the start of the try.

>@@ -3909,20 +3976,31 @@ ExtensionManager.prototype = {
>   },
> 
>   /**
>    * Write the Extensions List and the Startup Cache
>    * @param   needsRestart
>    *          true if the application needs to restart again, false otherwise.
>    */
>   _updateManifests: function(needsRestart) {
>-    // Write the Startup Cache (All Items, visible or not)
>-    StartupCache.write();
>-    // Write the Extensions Locations Manifest (Visible, enabled items)
>-    this._updateExtensionsManifest(needsRestart);
>+    // During startup we block flushing until the startup operations are all
>+    // complete to reduce file accesses that can trigger bug 431065
>+    if (gAllowFlush) {
>+      LOG("updateManifests");
nit: logging "updateManifests" is a tad vague. Perhaps ExtensionManager:_updateManifests - updating manifests?

>+      // Write the Startup Cache (All Items, visible or not)
>+      StartupCache.write();
>+      // Write the Extensions Locations Manifest (Visible, enabled items)
>+      this._updateExtensionsManifest();
>+    }
>+    else {
>+      gManifestNeedsFlush = true;
>+    }
>+
>+    // Now refresh the compatibility manifest.
nit: it is an old comment but this doesn't actually refresh the compatibility manifest. It does add the .autoreg file to notify nsAppRunner to refresh the compatibility manifest on the next startup.

>@@ -7543,16 +7616,30 @@ ExtensionsDataSource.prototype = {
>    */
>   setItemProperty: function (id, propertyArc, propertyValue) {
>     var item = getResourceForID(id);
>     this._setProperty(this._inner, item, propertyArc, propertyValue);
>     this.Flush();
>   },
> 
>   /**
>+   * Sets a number of properties on an item.
nit: Sets one or more properties for an item.

>+   * @param   id
>+   *          The GUID of the item
nit: we still have references to GUID from the old days but technically it is no longer a GUID. Please use ID or identifier in new comments.
http://en.wikipedia.org/wiki/Globally_Unique_Identifier

>+   * @param   properties
>+   *          A JS object which maps properties to values.
>+   */
>+  setItemProperties: function (id, properties) {
>+    var item = getResourceForID(id);
>+    for (var key in properties)
>+      this._setProperty(this._inner, item, EM_R(key), properties[key]);
>+    this.Flush();
>+  },
Nice!

>@@ -8705,16 +8794,22 @@ ExtensionsDataSource.prototype = {
> 
>   Init: function(uri) {
>   },
> 
>   Refresh: function(blocking) {
>   },
> 
>   Flush: function() {
>+    // During startup we block flushing until the startup operations are all
>+    // complete to reduce file accesses that can trigger bug 431065
nit: will there be times where we may want to block flushes during normal operations? If so, this comment might be better if it slightly were more generic.

r=me
Attachment #330926 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 9

10 years ago
Created attachment 331191 [details] [diff] [review]
patch rev 3

(In reply to comment #8)
> >-    // Write the Startup Cache (All Items, visible or not)
> >-    StartupCache.write();
> >-    // Write the Extensions Locations Manifest (Visible, enabled items)
> >-    this._updateExtensionsManifest(needsRestart);
> >+    // During startup we block flushing until the startup operations are all
> >+    // complete to reduce file accesses that can trigger bug 431065
> >+    if (gAllowFlush) {
> >+      LOG("updateManifests");
> nit: logging "updateManifests" is a tad vague. Perhaps
> ExtensionManager:_updateManifests - updating manifests?

Jut removed the log call altogether, it was a bit of code I was testing with that I didn't intend to make the final patch.

> >   Flush: function() {
> >+    // During startup we block flushing until the startup operations are all
> >+    // complete to reduce file accesses that can trigger bug 431065
> nit: will there be times where we may want to block flushes during normal
> operations? If so, this comment might be better if it slightly were more
> generic.

I've updated this. Hopefully though this covers the main cases and once we move to sqlite for the main datasource we won't have to worry about this problem except for the startup cache.

Fixed all other comments, carried over review.
Attachment #330926 - Attachment is obsolete: true
Attachment #331191 - Flags: review+
(Assignee)

Comment 10

10 years ago
Landed in changeset 8fda6cca7d24. There isn't really an automated way to test this as there is no behaviour change, other tests should just not fail.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Product: Firefox → Toolkit
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9.1a2
(Assignee)

Comment 11

10 years ago
Comment on attachment 331191 [details] [diff] [review]
patch rev 3

I would like to land this on branch. A fair number of people have been hitting issues with anti-virus tools breaking extension installation which this fixes. Since landing on the trunk I have not heard of any side effects
Attachment #331191 - Flags: approval1.9.0.2?
Comment on attachment 331191 [details] [diff] [review]
patch rev 3

Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #331191 - Flags: approval1.9.0.2? → approval1.9.0.2+
(Assignee)

Comment 13

10 years ago
Landed on branch

Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.289; previous revision: 1.288
done
Keywords: fixed1.9.0.2
You need to log in before you can comment on or make changes to this bug.