Closed Bug 697312 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

8 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mossop, Assigned: mossop)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
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?
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)
If it is a win either way that is great!
Assignee: nobody → dtownsend
Blocks: 586610
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #570011 - Flags: review?(bmcbride)
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.
Attachment #570011 - Flags: review?(bmcbride) → review+
(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.
Attached patch patch rev 2 (obsolete) — Splinter Review
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
Attachment #570011 - Attachment is obsolete: true
Attachment #570804 - Flags: review?(bmcbride)
Attachment #570804 - Flags: review?(benjamin)
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 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.
Attachment #570804 - Flags: review?(bmcbride) → review+
Comment on attachment 570804 [details] [diff] [review]
patch rev 2

I just pushed without the nsXREDirProvider changes
Attachment #570804 - Flags: review?(benjamin)
(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!)
Attached patch patch rev 3Splinter Review
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.
Attachment #570804 - Attachment is obsolete: true
Attachment #572670 - Flags: review?(bmcbride)
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.
Attachment #572670 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/7fc890b556bb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
No longer blocks: 586610
You need to log in before you can comment on or make changes to this bug.