994731 - Remove redundant in-memory cache from AddonRepository.jsm

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[:Irving Reid (No longer working on Firefox)]

Before bug 955389, AddonRepository data was stored in 'addons.sqlite', and was loaded into memory and stored in an in-memory cache (AddonRepository._addons).

After bug 955389, the entire contents of addons.json are read and parsed into memory when the database is loaded, so we don't need the extra complexity of maintaining a pseudo-cache on top of that in AddonRepository._addons.

Comment 1

[:Irving Reid (No longer working on Firefox)]

Oops, bug description should have referred to bug 853389.

Comment 2

[Alex Bardas :alexbardas]

Attachment: Removed _addons and _pendingCallback logic and use AddonDatabase.DB instead of the in-memory cache.

Comment 3

[Alex Bardas :alexbardas]

Attachment: Removed in-memory cache and use a promise to get the addon json from db.

Removed _addons and _pendingCallback logic. Use a promise to get the add-ons from db.

Comment 4

[Alex Bardas :alexbardas]

Irving: I've tried different approaches but I run into a concurrency problem when I debug using the Browser Toolbox inspector and test_AddonRepository_cache.js seems to fail.

I'll add another version of the patch right now, but tests still fail. Any thoughts?

Comment 5

[Alex Bardas :alexbardas]

Attachment: Remove in-memory cache from AddonRepository

Remove in-memory cache from _addons but keep _pendingCallbacks to keep track of all calls made to getCachedAddonByID. A certain test seems to fail.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8465110 [details] [diff] [review]
Remove in-memory cache from AddonRepository

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

It took me a little while to refresh my memory on this and remember what the tricky bits were...

If I recall correctly, this code (in particular, AddonRepository.getCachedAddonByID) actually has mixed sync/async behaviour - it will yield to the event loop and call back later if the AddonDatabase needs to be loaded, but it calls back synchronously if the DB is already loaded. There are test cases to verify this behaviour, using the test helper API at http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js#458. If check_cache() is called with aExpectedImmediately == True and getCachedAddonByID() yields to the event loop, the test will fail.

This means we need to either deliberately change the API (and the corresponding tests) to change getCachedAddonByID() to be always-async, or we need to carefully preserve the sometimes-async behaviour. For now, since we'd like to control scope on this, let's try to preserve the existing behaviour.

I'm going to write as if you're already familiar with ES6 Promise and fat-arrow function definitions; ask right away if something I say isn't clear.

A dxr search shows me that AddonDatabase.retrieveStoredData() is only ever called internally (by AddonRepository.getCachedAddonByID()), so we can go ahead and change its API or remove it without worrying about back compatibility. In general, we're trying to move from callback APIs to Promise. So, for retrieveStoredData(), we could either remove it completely and just call AddonDatabase.openConnection() directly to get a promise that resolves with the loaded database, or we could keep retrieveStoredData() and have it resolve with the db.addons field for convenience:

retrieveStoredData: function () {
  return this.openConnection().then(db => db.addons);
}


In AddonRepository.getCachedAddonByID(), I think we still need to keep the AddonRepository._addons field in order to preserve the sometimes-synchronous behaviour; since retrieveStoredData() isn't making a copy of that hash anymore, it's not wasting (a tiny bit of) memory to keep the reference. The logic we want is roughly:

if we have the DB already in this._addons:
    call back synchronously with the desired addon

if we don't have the DB:
    AddonDatabase.retrieveStoredData().then(addonList => {
       cache addonList in this._addons
       call back with the desired addon (the callback will be async, because Promise resolution always is)
    }

You can have multiple Promise.then() calls waiting for the same promise to resolve, and the Promise library will call them all for you, so if there are multiple calls to getCachedAddonByID() before the DB finishes loading, all the callbacks will be queued on that Promise.then(). That removes the need for keeping track of the _pendingCallbacks list.

Comment 7

[Alex Bardas :alexbardas]

Attachment: Refactored getCachedAddonById to use not need _pendingCallbacks and use promises

Remove _pendingCallbacks dependency and allow getCachedByAddonById to be both sync and async.

To make the _addons cache consistent, I've used a Map structure instead of an Object.

Comment 8

[Alex Bardas :alexbardas]

Thanks a lot for the comments :Irving, they were really helpful. 

To be honest, I think having getCachedAddonsById only async would be easier to maintain and will have no perf consequences.

Comment 9

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8465579 [details] [diff] [review]
Refactored getCachedAddonById to use not need _pendingCallbacks and use promises

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

A few little style nits to clean up, and this will be ready to land.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +579,2 @@
>      function getAddon(aAddons) {
> +      aCallback(aAddons.has(aId) ? aAddons.get(aId) : null);

since Map.get() returns 'undefined' if the key is not in the map, writing this as

aCallback(aAddons.get(aId) || null);

would prematurely optimize to only look up ID in the Map once.

@@ +581,4 @@
>      }
>  
>      if (this._addons == null) {
> +      AddonDatabase.retrieveStoredData().then((aAddons) => {

The parentheses around the formal parameter to the arrow function aren't needed if there is a single parameter; I usually leave them out because I like how it looks, I don't think Mozilla has an official style for that yet. You don't need to change this.

@@ +648,5 @@
>      yield new Promise((resolve, reject) =>
>        self._beginGetAddons(addonsToCache, {
>          searchSucceeded: function repopulateCacheInternal_searchSucceeded(aAddons) {
> +          self._addons = new Map();
> +          aAddons.forEach(function(aAddon) { self._addons.set(aAddon.id, aAddon); });

We like using Map ;->

While we're here, let's get rid of the Array.forEach call and use

for (let addon of aAddons) {
  self._addons.set(addon.id, addon);
}

Switching foreach() to for..of loops is a general style change we're making in the AddonManager code base as we go along.

@@ +691,5 @@
>        }
>  
>        self.getAddonsByIDs(aAddons, {
>          searchSucceeded: function cacheAddons_searchSucceeded(aAddons) {
> +          aAddons.forEach(function(aAddon) { self._addons.set(aAddon.id, aAddon); });

Again, switch to a for..of loop

@@ +1722,5 @@
>  
>    /**
> +   * Asynchronously retrieve all add-ons from the database
> +   * @return: Promise{null}
> +   *          Resolves when the add-ons are retrieved from the database

The comment isn't quite right; we need to document that the promise yields a usable value:

return: Promise{Map}
        Resolves with the contents of the add-on database when it has been loaded.

@@ +1727,5 @@
>     */
> +  retrieveStoredData: function (){
> +    return new Promise((resolve, reject) =>
> +      this.openConnection().then(db => resolve(db.addons)));
> +  },

You don't need to create a new Promise here; Promise.then() already does that for you. Ignoring error handling for the moment, the definition of "promise1.then(function)" is to return a new Promise 'promise2' that will:

1) wait for 'promise1' to fulfill
2) call 'function' on the value (if any) yielded by 'promise1' fulfilling
3) resolve promise2 with the result of the function call

so you really only need

return this.openConnection().then(db => db.addons);

Promises have quite a few subtle corner cases that make it easy to write relatively compact code, but it does take a while to get used to them...

Comment 10

[Alex Bardas :alexbardas]

Attachment: Refactored getCachedAddonById to use promises instead of _pendingCallbacks

Applied all irving's suggestions.

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 8466272 [details] [diff] [review]
Refactored getCachedAddonById to use promises instead of _pendingCallbacks

I pushed this patch to try: https://tbpl.mozilla.org/?tree=Try&rev=357e87ffe1b4

Comment 12

[Drew Willcoxon :adw]

Try is slow but so far looks OK.

Comment 13

[Drew Willcoxon :adw]

Er, sorry, I didn't notice Alex had requested review again.

Comment 14

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/02b52b8da6f4

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/02b52b8da6f4