Bug 1384236 - Cache l10n resources differently in L10nRegistry. r=Pike

Switch to cache FTLResources per FileSource. This allows us to minimize the memory impact of dynamic additions/removals of l10n resources to a context on fly. MozReview-Commit-ID: B9fxbkaU3oX
iaxat · Jun 22, 2018 · 05444f3 · 05444f3
1 parent 044a374
commit 05444f3
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 76 deletions.
diff --git a/intl/l10n/L10nRegistry.jsm b/intl/l10n/L10nRegistry.jsm
@@ -1,6 +1,6 @@
 const { AppConstants } = ChromeUtils.import("resource://gre/modules/AppConstants.jsm", {});
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm", {});
-const { MessageContext } = ChromeUtils.import("resource://gre/modules/MessageContext.jsm", {});
+const { MessageContext, FluentResource } = ChromeUtils.import("resource://gre/modules/MessageContext.jsm", {});
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 XPCOMUtils.defineLazyGlobalGetters(this, ["fetch"]);
 
@@ -76,10 +76,8 @@ XPCOMUtils.defineLazyGlobalGetters(this, ["fetch"]);
  * and will produce a new set of permutations placing the language pack provided resources
  * at the top.
  */
-
 const L10nRegistry = {
   sources: new Map(),
-  ctxCache: new Map(),
   bootstrap: null,
 
   /**
@@ -95,8 +93,29 @@ const L10nRegistry = {
       await this.bootstrap;
     }
     const sourcesOrder = Array.from(this.sources.keys()).reverse();
+    const pseudoNameFromPref = Services.prefs.getStringPref("intl.l10n.pseudo", "");
     for (const locale of requestedLangs) {
-      yield * generateContextsForLocale(locale, sourcesOrder, resourceIds);
+      for (const fetchPromises of generateResourceSetsForLocale(locale, sourcesOrder, resourceIds)) {
+        const ctx = await Promise.all(fetchPromises).then(
+          dataSets => {
+            const ctx = new MessageContext(locale, {
+              ...MSG_CONTEXT_OPTIONS,
+              transform: PSEUDO_STRATEGIES[pseudoNameFromPref],
+            });
+            for (const data of dataSets) {
+              if (data === null) {
+                return null;
+              }
+              ctx.addResource(data);
+            }
+            return ctx;
+          },
+          () => null
+        );
+        if (ctx !== null) {
+          yield ctx;
+        }
+      }
     }
   },
 
@@ -126,7 +145,6 @@ const L10nRegistry = {
       throw new Error(`Source with name "${source.name}" is not registered.`);
     }
     this.sources.set(source.name, source);
-    this.ctxCache.clear();
     Services.locale.setAvailableLocales(this.getAvailableLocales());
   },
 
@@ -158,21 +176,6 @@ const L10nRegistry = {
   },
 };
 
-/**
- * A helper function for generating unique context ID used for caching
- * MessageContexts.
- *
- * @param {String} locale
- * @param {Array} sourcesOrder
- * @param {Array} resourceIds
- * @returns {String}
- */
-function generateContextID(locale, sourcesOrder, resourceIds) {
-  const sources = sourcesOrder.join(",");
-  const ids = resourceIds.join(",");
-  return `${locale}|${sources}|${ids}`;
-}
-
 /**
  * This function generates an iterator over MessageContexts for a single locale
  * for a given list of resourceIds for all possible combinations of sources.
@@ -187,7 +190,7 @@ function generateContextID(locale, sourcesOrder, resourceIds) {
  * @param {Array} [resolvedOrder]
  * @returns {AsyncIterator<MessageContext>}
  */
-async function* generateContextsForLocale(locale, sourcesOrder, resourceIds, resolvedOrder = []) {
+function* generateResourceSetsForLocale(locale, sourcesOrder, resourceIds, resolvedOrder = []) {
   const resolvedLength = resolvedOrder.length;
   const resourcesLength = resourceIds.length;
 
@@ -208,14 +211,11 @@ async function* generateContextsForLocale(locale, sourcesOrder, resourceIds, res
     // If the number of resolved sources equals the number of resources,
     // create the right context and return it if it loads.
     if (resolvedLength + 1 === resourcesLength) {
-      const ctx = await generateContext(locale, order, resourceIds);
-      if (ctx !== null) {
-        yield ctx;
-      }
+      yield generateResourceSet(locale, order, resourceIds);
     } else if (resolvedLength < resourcesLength) {
       // otherwise recursively load another generator that walks over the
       // partially resolved list of sources.
-      yield * generateContextsForLocale(locale, sourcesOrder, resourceIds, order);
+      yield * generateResourceSetsForLocale(locale, sourcesOrder, resourceIds, order);
     }
   }
 }
@@ -346,35 +346,10 @@ const PSEUDO_STRATEGIES = {
  * @param {Array} resourceIds
  * @returns {Promise<MessageContext>}
  */
-function generateContext(locale, sourcesOrder, resourceIds) {
-  const ctxId = generateContextID(locale, sourcesOrder, resourceIds);
-  if (L10nRegistry.ctxCache.has(ctxId)) {
-    return L10nRegistry.ctxCache.get(ctxId);
-  }
-
-  const fetchPromises = resourceIds.map((resourceId, i) => {
+function generateResourceSet(locale, sourcesOrder, resourceIds) {
+  return resourceIds.map((resourceId, i) => {
     return L10nRegistry.sources.get(sourcesOrder[i]).fetchFile(locale, resourceId);
   });
-
-  const ctxPromise = Promise.all(fetchPromises).then(
-    dataSets => {
-      const pseudoNameFromPref = Services.prefs.getStringPref("intl.l10n.pseudo", "");
-      const ctx = new MessageContext(locale, {
-        ...MSG_CONTEXT_OPTIONS,
-        transform: PSEUDO_STRATEGIES[pseudoNameFromPref],
-      });
-      for (const data of dataSets) {
-        if (data === null) {
-          return null;
-        }
-        ctx.addMessages(data);
-      }
-      return ctx;
-    },
-    () => null
-  );
-  L10nRegistry.ctxCache.set(ctxId, ctxPromise);
-  return ctxPromise;
 }
 
 /**
@@ -454,15 +429,17 @@ class FileSource {
       if (this.cache[fullPath] === false) {
         return Promise.reject(`The source has no resources for path "${fullPath}"`);
       }
-      if (this.cache[fullPath].then) {
+      // `true` means that the file is indexed, but hasn't
+      // been fetched yet.
+      if (this.cache[fullPath] !== true) {
         return this.cache[fullPath];
       }
     } else if (this.indexed) {
         return Promise.reject(`The source has no resources for path "${fullPath}"`);
       }
     return this.cache[fullPath] = L10nRegistry.load(fullPath).then(
       data => {
-        return this.cache[fullPath] = data;
+        return this.cache[fullPath] = FluentResource.fromString(data);
       },
       err => {
         this.cache[fullPath] = false;

diff --git a/intl/l10n/Localization.jsm b/intl/l10n/Localization.jsm
@@ -263,7 +263,6 @@ class Localization {
       case "nsPref:changed":
         switch (data) {
           case "intl.l10n.pseudo":
-            L10nRegistry.ctxCache.clear();
             this.onChange();
         }
         break;

diff --git a/intl/l10n/MessageContext.jsm b/intl/l10n/MessageContext.jsm
@@ -1908,4 +1908,5 @@ class MessageContext {
 }
 
 this.MessageContext = MessageContext;
-var EXPORTED_SYMBOLS = ["MessageContext"];
+this.FluentResource = FluentResource;
+var EXPORTED_SYMBOLS = ["MessageContext", "FluentResource"];
diff --git a/intl/l10n/test/test_l10nregistry.js b/intl/l10n/test/test_l10nregistry.js
@@ -40,7 +40,6 @@ add_task(async function test_empty_resourceids() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -57,7 +56,6 @@ add_task(async function test_empty_sources() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -80,7 +78,6 @@ add_task(async function test_methods_calling() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -118,7 +115,6 @@ add_task(async function test_has_one_source() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -175,7 +171,6 @@ add_task(async function test_has_two_sources() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -203,7 +198,6 @@ add_task(async function test_indexed() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -244,7 +238,6 @@ add_task(async function test_override() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -282,7 +275,6 @@ add_task(async function test_updating() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -348,7 +340,6 @@ add_task(async function test_removing() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -378,15 +369,25 @@ add_task(async function test_missing_file() {
   // returns a single context
 
   let ctxs = L10nRegistry.generateContexts(["en-US"], ["test.ftl", "test2.ftl"]);
-  (await ctxs.next());
-  (await ctxs.next());
 
-  equal((await ctxs.next()).done, true);
+  // First permutation:
+  //   [platform, platform] - both present
+  let ctx1 = (await ctxs.next());
+  equal(ctx1.value.hasMessage("key"), true);
+
+  // Second permutation skipped:
+  //   [platform, app] - second missing
+  // Third permutation:
+  //   [app, platform] - both present
+  let ctx2 = (await ctxs.next());
+  equal(ctx2.value.hasMessage("key"), true);
 
+  // Fourth permutation skipped:
+  //   [app, app] - second missing
+  equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
@@ -447,6 +448,5 @@ add_task(async function test_parallel_io() {
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
 });
diff --git a/intl/l10n/test/test_localization.js b/intl/l10n/test/test_localization.js
@@ -43,7 +43,6 @@ add_task(async function test_methods_calling() {
   equal(values[1], "[en] Value3");
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
   Services.locale.setRequestedLocales(originalRequested);
 });
@@ -90,7 +89,6 @@ key = { PLATFORM() ->
     `${ known_platforms[AppConstants.platform].toUpperCase() } Value`));
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
 });
 
@@ -138,7 +136,6 @@ add_task(async function test_add_remove_resourceIds() {
   equal(values[1], "Value2");
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
   Services.locale.setRequestedLocales(originalRequested);
 });
diff --git a/intl/l10n/test/test_pseudo.js b/intl/l10n/test/test_pseudo.js
@@ -94,7 +94,6 @@ add_task(async function test_accented_works() {
   }
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalValues.load;
   Services.locale.setRequestedLocales(originalValues.requested);
 });
@@ -126,7 +125,6 @@ add_task(async function test_unavailable_strategy_works() {
 
   Services.prefs.setStringPref("intl.l10n.pseudo", "");
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalValues.load;
   Services.locale.setRequestedLocales(originalValues.requested);
 });