Last Comment Bug 692712 - Pre-filter recommended add-ons JSON instead of filtering at startup
: Pre-filter recommended add-ons JSON instead of filtering at startup
Status: RESOLVED FIXED
[mobilestartupshrink][QA?]
: perf
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
Depends on:
Blocks: 673253
  Show dependency treegraph
 
Reported: 2011-10-06 22:21 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2011-11-08 17:31 PST (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (6.46 KB, patch)
2011-10-07 09:40 PDT, Mark Finkle (:mfinkle) (use needinfo?)
wjohnston2000: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-10-06 22:21:36 PDT
Filtering at startup, in about:home requires the add-on manager code, and that requires SQLite access.

We could pre-filter the JSON when we save the file and when we add/remove an add-on. Those happen much less often than startup.

(Thanks Dietrich and Mossop for the idea)
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-06 22:23:46 PDT
Note to others: We store the recommended add-ons in a JSON file so we can display them in the startup page. We filter the list to remove any add-ons you already have installed.
Comment 2 Dave Townsend [:mossop] 2011-10-06 22:25:46 PDT
You could cache the unfiltered and filtered lists. On startup call AddonManager.getStartupChanges for STARTUP_CHANGE_INSTALLED and STARTUP_CHANGE_UNINSTALLED. If either returns a non-empty array then the set of installed add-ons has changed so you can re-request the list of add-ons and filter again (in this case the sqlite DB will already have been loaded).
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-07 07:50:44 PDT
(In reply to Dave Townsend (:Mossop) from comment #2)
> You could cache the unfiltered and filtered lists. On startup call
> AddonManager.getStartupChanges for STARTUP_CHANGE_INSTALLED and
> STARTUP_CHANGE_UNINSTALLED. If either returns a non-empty array then the set
> of installed add-ons has changed so you can re-request the list of add-ons
> and filter again (in this case the sqlite DB will already have been loaded).

Checking for these doesn't seem to catch restartless addons
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-07 09:40:03 PDT
Created attachment 565561 [details] [diff] [review]
patch

This patch makes a few changes to the way we use the recommended add-ons cache:

* Avoids using AddonManager to filter the cache, which avoids causing the add-ons SQLite DB from being opened during the start page activity.
* Filters the cache when fetching it
* Reduces the size of the raw cache file by only storing data fields we actuall (or might actually) use. This reduces my cache from 8.2KB to 654B, making it faster to save and load.
* Forces a cache refresh whenever an add-on is installed. We could do it for uninstalling add-ons too, but this seem like less of a priority.

The
Comment 5 Wesley Johnston (:wesj) 2011-10-07 14:51:28 PDT
Comment on attachment 565561 [details] [diff] [review]
patch

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

Cool. Sorry for the delay. Just had to dig through and remember what's going on here.

Just to note, this will only prevent us from hitting the AddonsManager when we already have a cache. If you install an addon or if its first run, or the cache was deleted for some other reason, we'll still have to initialize the list. I half wonder if we'd be better to avoid it on firstrun as well, when we may have to create databases?

::: mobile/chrome/content/extensions.js
@@ +1066,5 @@
>  
>      ExtensionsView.showAlert(strings.GetStringFromName(stringName), !aNeedsRestart);
>    },
> +
> +  _refeshRecommendedCache: function xpidm_refeshRecommendedCache() {

This doesn't refresh the cache. It deletes it right? I think we should name this something more in line with what it does. clearRecommendedCache maybe? And then maybe quick comment about why we're deleting it I guess.

::: mobile/components/AddonUpdateService.js
@@ +190,5 @@
>  
> +    // Filter addons already installed
> +    AddonManager.getAllAddons(function(aAllAddons) {
> +      let addons = aAddons.filter(function(addon) {
> +        for (let i =0; i < aAllAddons.length; i++)

nit:missing space
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-07 19:15:22 PDT
Fixed nits:
https://hg.mozilla.org/mozilla-central/rev/194720ba054f
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-07 20:50:07 PDT
Comment on attachment 565561 [details] [diff] [review]
patch

This change is not a 1-liner, but it is simple. It moves code to filter a list of add-ons read from disk on the home screen to the place where we save the list of add-ons to disk.

This reduces the amount of work done at startup and avoids opening the AddonManager SQLite DB during startup.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-13 06:33:39 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/f41ea62c6d4c

Note You need to log in before you can comment on or make changes to this bug.