Bug 1384236 - Cache l10n resources differently in L10nRegistry. r?pike draft
authorZibi Braniecki <zbraniecki@mozilla.com>
Fri, 22 Jun 2018 12:03:24 -0700
changeset 818503 2b23d72b6ae6dac6bb73e74790f4a03fdcd44e4e
parent 816980 f6ed958c7cbb522f3ee8d7791793db8bb71e227b
child 818620 0e1ff5d0f31501977f2ab84ba1d07dab76eefc18
push id116277
push userbmo:gandalf@aviary.pl
push dateSat, 14 Jul 2018 13:06:37 +0000
reviewerspike
bugs1384236
milestone63.0a1
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
intl/l10n/L10nRegistry.jsm
intl/l10n/Localization.jsm
intl/l10n/MessageContext.jsm
intl/l10n/test/test_l10nregistry.js
intl/l10n/test/test_localization.js
intl/l10n/test/test_pseudo.js
--- a/intl/l10n/L10nRegistry.jsm
+++ b/intl/l10n/L10nRegistry.jsm
@@ -1,11 +1,11 @@
 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"]);
 
 /**
  * L10nRegistry is a localization resource management system for Gecko.
  *
  * It manages the list of resource sources provided with the app and allows
  * for additional sources to be added and updated.
@@ -71,37 +71,56 @@ XPCOMUtils.defineLazyGlobalGetters(this,
  *
  * This allows the localization API to consume the MessageContext and lazily fallback
  * on the next in case of a missing string or error.
  *
  * If during the life-cycle of the app a new source is added, the generator can be called again
  * 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,
 
   /**
    * Based on the list of requested languages and resource Ids,
    * this function returns an lazy iterator over message context permutations.
    *
    * @param {Array} requestedLangs
    * @param {Array} resourceIds
    * @returns {AsyncIterator<MessageContext>}
    */
   async* generateContexts(requestedLangs, resourceIds) {
     if (this.bootstrap !== null) {
       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;
+        }
+      }
     }
   },
 
   /**
    * Adds a new resource source to the L10nRegistry.
    *
    * @param {FileSource} source
    */
@@ -121,17 +140,16 @@ const L10nRegistry = {
    *
    * @param {FileSource} source
    */
   updateSource(source) {
     if (!this.sources.has(source.name)) {
       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());
   },
 
   /**
    * Removes a source from the L10nRegistry.
    *
    * @param {String} sourceId
    */
@@ -154,45 +172,30 @@ const L10nRegistry = {
         locales.add(locale);
       }
     }
     return Array.from(locales);
   },
 };
 
 /**
- * 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.
  *
  * This function is called recursively to generate all possible permutations
  * and uses the last, optional parameter, to pass the already resolved
  * sources order.
  *
  * @param {String} locale
  * @param {Array} sourcesOrder
  * @param {Array} 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;
 
   // Inside that loop we have a list of resources and the sources for them, like this:
   //   ['test.ftl', 'menu.ftl', 'foo.ftl']
   //   ['app', 'platform', 'app']
   for (const sourceName of sourcesOrder) {
     const order = resolvedOrder.concat(sourceName);
@@ -203,24 +206,21 @@ async function* generateContextsForLocal
     // have to perform the I/O to learn.
     if (L10nRegistry.sources.get(sourceName).hasFile(locale, resourceIds[resolvedOrder.length]) === false) {
       continue;
     }
 
     // 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);
     }
   }
 }
 
 const MSG_CONTEXT_OPTIONS = {
   // Temporarily disable bidi isolation due to Microsoft not supporting FSI/PDI.
   // See bug 1439018 for details.
   useIsolating: Services.prefs.getBoolPref("intl.l10n.enable-bidi-marks", false),
@@ -341,45 +341,20 @@ const PSEUDO_STRATEGIES = {
  * This allows the caller to be an async generator without using
  * try/catch clauses.
  *
  * @param {String} locale
  * @param {Array} sourcesOrder
  * @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;
 }
 
 /**
  * This is a basic Source for L10nRegistry.
  * It registers its own locales and a pre-path, and when asked for a file
  * it attempts to download and cache it.
  *
  * The Source caches the downloaded files so any consecutive loads will
@@ -449,25 +424,27 @@ class FileSource {
     }
 
     const fullPath = this.getPath(locale, path);
 
     if (this.cache.hasOwnProperty(fullPath)) {
       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;
         return Promise.reject(err);
       }
     );
   }
 }
--- a/intl/l10n/Localization.jsm
+++ b/intl/l10n/Localization.jsm
@@ -258,17 +258,16 @@ class Localization {
   observe(subject, topic, data) {
     switch (topic) {
       case "intl:app-locales-changed":
         this.onChange();
         break;
       case "nsPref:changed":
         switch (data) {
           case "intl.l10n.pseudo":
-            L10nRegistry.ctxCache.clear();
             this.onChange();
         }
         break;
       default:
         break;
     }
   }
 
--- a/intl/l10n/MessageContext.jsm
+++ b/intl/l10n/MessageContext.jsm
@@ -1903,9 +1903,10 @@ class MessageContext {
       this._intls.set(ctor, cache);
     }
 
     return cache[id];
   }
 }
 
 this.MessageContext = MessageContext;
-var EXPORTED_SYMBOLS = ["MessageContext"];
+this.FluentResource = FluentResource;
+var EXPORTED_SYMBOLS = ["MessageContext", "FluentResource"];
--- a/intl/l10n/test/test_l10nregistry.js
+++ b/intl/l10n/test/test_l10nregistry.js
@@ -35,34 +35,32 @@ add_task(async function test_empty_resou
   const ctxs = L10nRegistry.generateContexts(["en-US"], []);
 
   const done = (await ctxs.next()).done;
 
   equal(done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * Test that passing empty sources list works.
  */
 add_task(async function test_empty_sources() {
   fs = {};
 
   const ctxs = L10nRegistry.generateContexts(["en-US"], []);
 
   const done = (await ctxs.next()).done;
 
   equal(done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test tests generation of a proper context for a single
  * source scenario
  */
 add_task(async function test_methods_calling() {
   fs = {
@@ -75,17 +73,16 @@ add_task(async function test_methods_cal
   const ctxs = L10nRegistry.generateContexts(["en-US"], ["/browser/menu.ftl"]);
 
   const ctx = (await ctxs.next()).value;
 
   equal(ctx.hasMessage("key"), true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that the public methods return expected values
  * for the single source scenario
  */
 add_task(async function test_has_one_source() {
   let oneSource = new FileSource("app", ["en-US"], "./app/data/locales/{locale}/");
@@ -113,17 +110,16 @@ add_task(async function test_has_one_sou
   // returns no contexts for missing locale
 
   ctxs = L10nRegistry.generateContexts(["pl"], ["test.ftl"]);
 
   equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that public methods return expected values
  * for the dual source scenario.
  */
 add_task(async function test_has_two_sources() {
   let oneSource = new FileSource("platform", ["en-US"], "./platform/data/locales/{locale}/");
@@ -170,17 +166,16 @@ add_task(async function test_has_two_sou
   equal(ctx1.hasMessage("key"), true);
   let msg1 = ctx1.getMessage("key");
   equal(ctx1.format(msg1), "platform value");
 
   equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that behavior specific to the IndexedFileSource
  * works correctly.
  *
  * In particular it tests that IndexedFileSource correctly returns
  * missing files as `false` instead of `undefined`.
@@ -198,17 +193,16 @@ add_task(async function test_indexed() {
   equal(L10nRegistry.sources.has("langpack-pl"), true);
 
   equal(oneSource.getPath("pl", "test.ftl"), "/data/locales/pl/test.ftl");
   equal(oneSource.hasFile("pl", "test.ftl"), true);
   equal(oneSource.hasFile("pl", "missing.ftl"), false);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test checks if the correct order of contexts is used for
  * scenarios where a new file source is added on top of the default one.
  */
 add_task(async function test_override() {
   let fileSource = new FileSource("app", ["pl"], "/app/data/locales/{locale}/");
@@ -239,17 +233,16 @@ add_task(async function test_override() 
   equal(ctx1.hasMessage("key"), true);
   let msg1 = ctx1.getMessage("key");
   equal(ctx1.format(msg1), "value");
 
   equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that new contexts are returned
  * after source update.
  */
 add_task(async function test_updating() {
   let oneSource = new IndexedFileSource("langpack-pl", ["pl"], "/data/locales/{locale}/", [
@@ -277,17 +270,16 @@ add_task(async function test_updating() 
   equal(L10nRegistry.sources.size, 1);
   ctxs = L10nRegistry.generateContexts(["pl"], ["test.ftl"]);
   ctx0 = (await ctxs.next()).value;
   msg0 = ctx0.getMessage("key");
   equal(ctx0.format(msg0), "new value");
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that generated contexts return correct values
  * after sources are being removed.
  */
 add_task(async function test_removing() {
   let fileSource = new FileSource("app", ["pl"], "/app/data/locales/{locale}/");
@@ -343,17 +335,16 @@ add_task(async function test_removing() 
 
   equal(L10nRegistry.sources.size, 0);
 
   ctxs = L10nRegistry.generateContexts(["pl"], ["test.ftl"]);
   equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that the logic works correctly when there's a missing
  * file in the FileSource scenario.
  */
 add_task(async function test_missing_file() {
   let oneSource = new FileSource("app", ["en-US"], "./app/data/locales/{locale}/");
@@ -373,25 +364,35 @@ add_task(async function test_missing_fil
   equal(L10nRegistry.sources.size, 2);
   equal(L10nRegistry.sources.has("app"), true);
   equal(L10nRegistry.sources.has("platform"), true);
 
 
   // returns a single context
 
   let ctxs = L10nRegistry.generateContexts(["en-US"], ["test.ftl", "test2.ftl"]);
-  (await ctxs.next());
-  (await ctxs.next());
+
+  // 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();
 });
 
 /**
  * This test verifies that each file is that all files requested
  * by a single context are fetched at the same time, even
  * if one I/O is slow.
  */
 add_task(async function test_parallel_io() {
@@ -442,11 +443,10 @@ add_task(async function test_parallel_io
   equal((await ctxs.next()).done, true);
 
   // When requested again, the cache should make the load operation not
   // increase the fetchedIndex count
   L10nRegistry.generateContexts(["en-US"], ["test.ftl", "test2.ftl", "slow-file.ftl"]);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
 });
--- a/intl/l10n/test/test_localization.js
+++ b/intl/l10n/test/test_localization.js
@@ -38,17 +38,16 @@ add_task(async function test_methods_cal
   ], generateMessages);
 
   let values = await l10n.formatValues([{id: "key"}, {id: "key2"}]);
 
   equal(values[0], "[de] Value2");
   equal(values[1], "[en] Value3");
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
   Services.locale.setRequestedLocales(originalRequested);
 });
 
 add_task(async function test_builtins() {
   const { L10nRegistry, FileSource } =
     ChromeUtils.import("resource://gre/modules/L10nRegistry.jsm", {});
 
@@ -85,17 +84,16 @@ key = { PLATFORM() ->
   ], generateMessages);
 
   let values = await l10n.formatValues([{id: "key"}]);
 
   ok(values[0].includes(
     `${ known_platforms[AppConstants.platform].toUpperCase() } Value`));
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
 });
 
 add_task(async function test_add_remove_resourceIds() {
   const { L10nRegistry, FileSource } =
     ChromeUtils.import("resource://gre/modules/L10nRegistry.jsm", {});
 
   const fs = {
@@ -133,12 +131,11 @@ add_task(async function test_add_remove_
   l10n.removeResourceIds(["/browser/menu.ftl"]);
 
   values = await l10n.formatValues([{id: "key1"}, {id: "key2"}]);
 
   equal(values[0], undefined);
   equal(values[1], "Value2");
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
   Services.locale.setRequestedLocales(originalRequested);
 });
--- a/intl/l10n/test/test_pseudo.js
+++ b/intl/l10n/test/test_pseudo.js
@@ -89,17 +89,16 @@ add_task(async function test_accented_wo
     let message = (await l10n.formatMessages([{id: "key"}]))[0];
 
     ok(message.value.includes("This is a single message"));
     ok(message.attributes[0].value.includes("This is a tooltip"));
     equal(message.attributes[1].value, "f");
   }
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalValues.load;
   Services.locale.setRequestedLocales(originalValues.requested);
 });
 
 /**
  * This test verifies that setting a bogus pseudo locale
  * strategy doesn't break anything.
  */
@@ -121,12 +120,11 @@ add_task(async function test_unavailable
 
     ok(message.value.includes("This is a single message"));
     ok(message.attributes[0].value.includes("This is a tooltip"));
     equal(message.attributes[1].value, "f");
   }
 
   Services.prefs.setStringPref("intl.l10n.pseudo", "");
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalValues.load;
   Services.locale.setRequestedLocales(originalValues.requested);
 });