Closed Bug 697246 Opened 8 years ago Closed 8 years ago

Defer creating the extensions database until it is actually necessary


(Toolkit :: Add-ons Manager, defect)

8 Branch
Not set





(Reporter: mossop, Assigned: mossop)


(Blocks 1 open bug)



(2 files, 2 obsolete files)

In a profile where there are no extensions present there is no need to actually create the extensions database. This is pretty much the common case on mobile so we're wasting a lot of time opening the db and creating its schema for nothing.

I have a patch that only creates the database on startup if there is no database file and there are add-ons present. Attempts to query from the database return correctly empty when there is no database file and attempting to add to the database will then create the database and schema.
Blocks: 696141
Attached patch patch rev 1 (obsolete) — Splinter Review
This stops us creating a database when there are no add-ons installed and makes all of the calls on XPIDatabase that might be called without having previously opened a database return sane results in that case
Attachment #570014 - Flags: review?(bmcbride)
Comment on attachment 570014 [details] [diff] [review]
patch rev 1

Review of attachment 570014 [details] [diff] [review]:

Looks good, aside from one thing noted below. But this is begging to be tested.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +1805,5 @@
>    getInstallLocationStates: function XPI_getInstallLocationStates() {
> +    let state = {
> +      haveAddons: false,
> +      locationStates: []
> +    };

state.haveAddons seems redundant, as it will always be equal to |state.locationStates.length == 0|
Attachment #570014 - Flags: review?(bmcbride) → review-
Attached patch patch rev 2 (obsolete) — Splinter Review
Well yes that just makes things simpler, who wants that!

I added a couple of checks that the database is missing and gets creating when expected, the fact that all the other tests run basically confirms that the DB is always getting created when it is needed. Did you have any other specific checks in mind?
Attachment #570014 - Attachment is obsolete: true
Attachment #570796 - Flags: review?(bmcbride)
Attachment #570796 - Flags: review?(bmcbride) → review+
Between the patch and and the one in bug 697312, there are a couple more file stats (via nsILocalFile::exists()). Not sure if those would be enough to cause that kind of regression or not (I'd expect not, but...). 

Anyway, could probably get rid of one or two calls to .exists() - namely where it checks if the file exists before removing (just removing and catching any exception is quicker than checking if the file exists first). Also the .exists() call in openConnection(). 

Could probably just make one exists() call for each file, and cache the result too. Since the code knows when it's going to create the files.
(In reply to Ed Morley [:edmorley] from comment #5)
> Backout:

The non-empty-changeset version:

(Had entered the changesets in order old to new instead of vice versa, in Mak's backout script and failed to check hg outgoing wasn't empty, doh!)
Ok I think this patch is actually safe (actually showing a win on my system) so I'll probably reland it if the try results match up with that. I think bug 697312 was causing the perf hit.
Attached patch patch rev 3Splinter Review
Tests showed that this really wasn't the cause of the perf hit, any change was well within noise limits, I did however reduce the number of stats to the db file anyway while I was updating it.
Attachment #572668 - Flags: review?(bmcbride)
Attachment #572668 - Flags: review?(bmcbride) → review+
Attached patch follow-up fixSplinter Review
This follow-up fix is required for test failures on Windows. There we have a stat cache so the first time we test if dbfile exists we retain the entire file stat. test_corrupt starts the manager and then replaces the file with a directory. When the add-ons manager comes to open the db afterwards it fails correctly but when it attempts to remove it the platform code thinks it is trying to remove a file, not a directory and so things go wrong. This code just replaces the file with a directory before starting the manager again.
Attachment #570796 - Attachment is obsolete: true
Attachment #573598 - Flags: review?(bmcbride)
Attachment #573598 - Flags: review?(bmcbride) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 879587
You need to log in before you can comment on or make changes to this bug.