697312 - Don't write out extensions.ini if there are no extensions

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

On first run when there are no extensions installed we still write out extensions.ini. We should just make sure we can work without one and not incur the cost (~30ms on mobile) of this.

Comment 1

[Bill Gianopoulos [:WG9s]]

OK.  But this is a penalty only on first run.  So, if taking the special extensions.ini file is absent imposes a startup penalty on future runs, is this necessarily really a performance win?

Comment 2

[Dave Townsend [:mossop]]

First run is still a pretty important metric and while I can accept it being larger than normal run there is no loss to doing this (in fact with no extensions.ini to be parsed this ought to speed up subsequent runs slightly too)

Comment 3

[Bill Gianopoulos [:WG9s]]

If it is a win either way that is great!

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This depends on the patch in bug 697246.

This makes us delete the extensions.ini if it would be empty, it also stops us including the default theme in it since there is no chrome.manifest to parse there.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 570011 [details] [diff] [review]
patch rev 1

Review of attachment 570011 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5484,5 @@
>      // The selected skin may come from an inactive theme (the default theme
>      // when a lightweight theme is applied for example)
>      text += "\r\n[ThemeDirs]\r\n";
>  
> +    stmt = null;

Nit: No need to null this.

Comment 6

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #5)
> Comment on attachment 570011 [details] [diff] [review] [diff] [details] [review]
> patch rev 1
> 
> Review of attachment 570011 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/XPIProvider.jsm
> @@ +5484,5 @@
> >      // The selected skin may come from an inactive theme (the default theme
> >      // when a lightweight theme is applied for example)
> >      text += "\r\n[ThemeDirs]\r\n";
> >  
> > +    stmt = null;
> 
> Nit: No need to null this.

I don't think that's true, both of the following if statements could fail if there are no themes to write to extensions.ini at all, we need to know if that happens.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated from the patch in bug 697246 and added a couple of additional checks.

Benjamin, do you want to look at the small change to nsXREDirProvider

Comment 8

[Benjamin Smedberg]

Comment on attachment 570804 [details] [diff] [review]
patch rev 2

I don't see any reason to check whether extensions.ini exists before trying to open it: it's an extra stat for no particular good reason: just try to open it. See bug 521264. That is to say, I don't think the change in nsXREDirProvider is needed at all.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 570804 [details] [diff] [review]
patch rev 2

Review of attachment 570804 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dave Townsend (:Mossop) from comment #6)
> > Nit: No need to null this.
> 
> I don't think that's true, both of the following if statements could fail if
> there are no themes to write to extensions.ini at all, we need to know if
> that happens.

Oops, yea, I read that else-if as an else.

Comment 10

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f551298a760

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 570804 [details] [diff] [review]
patch rev 2

I just pushed without the nsXREDirProvider changes

Comment 12

[Ed Morley [:emorley]]

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

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 13

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #12)
> 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 14

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This was a terrible perf hit on desktop because it accidentally caused us to open the database on every startup when normally we don't open it at all. What happened is we would do the first run, build the database then attempt to write out extensions.ini. Because I made us not write the default theme in there to fix bug 586610 there was nothing to write so it didn't write out a file. Then on the next startup (and every startup for the Ts test) we'd start, see there was an extension installed (the default theme), see there wasn't an extensions.ini and so try to write it out, which involved opening the DB.

Couldn't come up with a straightforward solution there right now so instead I've just abandoned the fix for bug 586610 for now.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 572670 [details] [diff] [review]
patch rev 3

Review of attachment 572670 [details] [diff] [review]:
-----------------------------------------------------------------

Pity, as this means desktop won't see any improvement. Still, I guess it was always mostly for mobile anyway.

Comment 16

[Phil Ringnalda (:philor)]

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

Comment 17

[Dave Townsend [:mossop]]

Back on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc890b556bb

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7fc890b556bb