752868 - Outdated locales aren't being removed from extensions.sqlite, causing bad performance

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Wladimir Palant]

Populating the addons list got pretty bad in my development profile lately, taking more than 20 seconds for 23 add-ons. I started JavaScript Deobfuscator to figure out what's holding it - and it pointed to XPIDatabase._readLocaleStrings as the operation being executed. Turned out, my extensions.sqlite file was almost 50 MB large, vacuuming only brought it down to 25 MB. Here are the stats from it:

sqlite> select count(*) from addon;
23
sqlite> select count(*) from addon_locale;
162
sqlite> select count(*) from locale;
10271
sqlite> select count(*) from locale_strings;
651031

Creating the database from scratch brought the size down to 800 kB (merely 346 locales left), the list is being populated within a second.

That's a development profile with tons of extension installations daily, being used with Firefox nightlies. So I also checked my regular profile which is only used with stable Firefox releases (currently Firefox 12), to make sure that this wasn't some temporary nightly glitch fixed long ago. The database size there is 2 MB:

sqlite> select count(*) from addon;
19
sqlite> select count(*) from addon_locale;
126
sqlite> select count(*) from locale;
680
sqlite> select count(*) from locale_strings;
32814

The issue isn't quite as bad here but the number of locales still seems way off. Looks like removing old locales on update doesn't work, or at least not always.

Comment 1

[Dave Townsend [:mossop]]

Strange, we rely on sqlite triggers to clean up the other tables when we remove an entry for addon (even updates are just a remove/add at the moment): http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#4694

I wonder if the triggers aren't working in some way?

Comment 2

[Dave Townsend [:mossop]]

Huh, the table locale_strings has no index, I bet adding one for locale_id would make a big difference to the speed here, regardless of whether we're accidentally leaving junk behind.

Comment 3

[Wladimir Palant]

Two days later my extensions.sqlite is at 12 MB again - Adblock Plus has way too many strings. The triggers seem to work fine in general, e.g. addon_locale doesn't have any stale entries. It's only the trigger supposed to remove from locale table that isn't working. Running the following SQL statements fixes things:

> delete from locale where id not in (select locale_id from addon_locale) and id not in (select defaultLocale from addon);
> vacuum;

Note that locale_strings table is emptied automatically meaning that the corresponding trigger works. There is only some issue with the delete_addon_locale trigger.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I think locale_strings is *not* getting emptied properly also. For Adblock Plus, all the rows in locale_strings are associated with the default locale - that's removed from the locale table by the delete_addon trigger. If there were strings from another locale in there, those wouldn't be removed because the delete_addon_locale trigger is misbehaving (not sure why, yet).

Additionally, addAddonMetadata() seems to be adding locales to the DB at least twice, via insertLocale():
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#5361
Not sure if that's related yet.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride (:Unfocused) from comment #4)
> Additionally, addAddonMetadata() seems to be adding locales to the DB at
> least twice, via insertLocale():
> https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> XPIProvider.jsm#5361
> Not sure if that's related yet.

Oh, duh, it is directly related. The triggers are working fine, but they only delete rows with an ID matching rows deleted from addon_locale. Since the locale table has extra rows with IDs that reference rows in addon_locale that don't exist, those extra rows are what don't get removed.

Comment 6

[Wladimir Palant]

A note: I've added the query from comment 3 to Extension Auto-Installer 1.2 (released it a few minutes ago). It will now remove unused locales every time an add-on is installed through it - things slow down way too soon otherwise.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Yes, I'm kinda abusing DB_SCHEMA here - but revving that is a convenient way to clean the database.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #7)
> Created attachment 624695 [details] [diff] [review]
> Patch v1
> 
> Yes, I'm kinda abusing DB_SCHEMA here - but revving that is a convenient way
> to clean the database.

That's fine, but since we're revving the schema can you also add an index on locale_strings for locale_id.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 624695 [details] [diff] [review]
Patch v1

The code that is here looks good but please add the index before landing.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, duh (again). Pushed with a new index for locale_strings:

https://hg.mozilla.org/integration/fx-team/rev/4ae03007e811

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4ae03007e811