697246 - Defer creating the extensions database until it is actually necessary

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

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

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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|

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 2

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?

Comment 4

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b31507c8ca17

Comment 5

[Ed Morley [:emorley]]

Merged to m-c as:
https://hg.mozilla.org/mozilla-central/rev/b31507c8ca17

However backed out of inbound due to 25-30% Ts regression on several platforms:
http://graphs-new.mozilla.org/graph.html#tests=[[83,131,14]]&sel=1320342484377.203,1320398724082&displayrange=7&datatype=running

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e50dd40a55

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 7

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #5)
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/57e50dd40a55

The non-empty-changeset version:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c535d936df7f

(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!)

Comment 8

[Dave Townsend [:mossop]]

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.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 3

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.

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ae523739112e for Win xpcshell orange

Comment 11

[Dave Townsend [:mossop]]

Attachment: follow-up fix

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.

Comment 12

[Dave Townsend [:mossop]]

Hoping third times the charm: https://hg.mozilla.org/integration/mozilla-inbound/rev/736d3db54c41

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/736d3db54c41