Bug 1594405 - Cache mozjexl evaluation results to reuse between frequent calls. r=k88hudson a=pascalc FIREFOX_71_0_BUILD4
authorAndrei Oprea <andrei.br92@gmail.com>
Thu, 28 Nov 2019 14:58:24 +0100
changeset 563498 ee54407d806ffbfd10f2c05f54e729e12a704ccd
parent 563497 a514ead5cb170c7d66faf354c5d1a403c8938989
child 563499 e92c4aeac6c046e5f6e3954ffee9fcc9d19045dd
push id2203
push userarchaeopteryx@coole-files.de
push dateThu, 28 Nov 2019 22:17:51 +0000
treeherdermozilla-release@ee54407d806f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersk88hudson, pascalc
bugs1594405
milestone71.0
Bug 1594405 - Cache mozjexl evaluation results to reuse between frequent calls. r=k88hudson a=pascalc Differential Revision: https://phabricator.services.mozilla.com/D55112
browser/components/newtab/lib/ASRouter.jsm
browser/components/newtab/lib/ASRouterTargeting.jsm
browser/components/newtab/test/unit/asrouter/ASRouter.test.js
browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js
--- a/browser/components/newtab/lib/ASRouter.jsm
+++ b/browser/components/newtab/lib/ASRouter.jsm
@@ -104,16 +104,20 @@ const STARTPAGE_VERSION = "6";
 // Remote Settings
 const RS_SERVER_PREF = "services.settings.server";
 const RS_MAIN_BUCKET = "main";
 const RS_COLLECTION_L10N = "ms-language-packs"; // "ms" stands for Messaging System
 const RS_PROVIDERS_WITH_L10N = ["cfr", "cfr-fxa"];
 const RS_FLUENT_VERSION = "v1";
 const RS_FLUENT_RECORD_PREFIX = `cfr-${RS_FLUENT_VERSION}`;
 const RS_DOWNLOAD_MAX_RETRIES = 2;
+// This is the list of providers for which we want to cache the targeting
+// expression result and reuse between calls. Cache duration is defined in
+// ASRouterTargeting where evaluation takes place.
+const JEXL_PROVIDER_CACHE = new Set(["snippets"]);
 
 // To observe the app locale change notification.
 const TOPIC_INTL_LOCALE_CHANGED = "intl:app-locales-changed";
 // To observe the pref that controls if ASRouter should use the remote Fluent files for l10n.
 const USE_REMOTE_L10N_PREF =
   "browser.newtabpage.activity-stream.asrouter.useRemoteL10n";
 
 /**
@@ -1198,43 +1202,45 @@ class _ASRouter {
         return trailheadInterrupt;
       },
       get trailheadTriplet() {
         return trailheadTriplet;
       },
     };
   }
 
-  _findAllMessages(candidateMessages, trigger) {
+  _findAllMessages(candidateMessages, trigger, { shouldCache = false } = {}) {
     const messages = candidateMessages.filter(m =>
       this.isBelowFrequencyCaps(m)
     );
     const context = this._getMessagesContext();
 
     return ASRouterTargeting.findAllMatchingMessages({
       messages,
       trigger,
       context,
       onError: this._handleTargetingError,
+      shouldCache,
     });
   }
 
-  _findMessage(candidateMessages, trigger) {
+  _findMessage(candidateMessages, trigger, { shouldCache = false } = {}) {
     const messages = candidateMessages.filter(m =>
       this.isBelowFrequencyCaps(m)
     );
     const context = this._getMessagesContext();
 
     // Find a message that matches the targeting context as well as the trigger context (if one is provided)
     // If no trigger is provided, we should find a message WITHOUT a trigger property defined.
     return ASRouterTargeting.findMatchingMessage({
       messages,
       trigger,
       context,
       onError: this._handleTargetingError,
+      shouldCache,
     });
   }
 
   async evaluateExpression(target, { expression, context }) {
     const channel = target || this.messageChannel;
     let evaluationStatus;
     try {
       evaluationStatus = {
@@ -1631,34 +1637,38 @@ class _ASRouter {
       }
       if (triggerId && m.trigger.id !== triggerId) {
         return false;
       }
 
       return true;
     });
 
+    const shouldCache = msgs.every(m => JEXL_PROVIDER_CACHE.has(m.provider));
+
     if (returnAll) {
       return this._findAllMessages(
         msgs,
         triggerId && {
           id: triggerId,
           param: triggerParam,
           context: triggerContext,
-        }
+        },
+        { shouldCache }
       );
     }
 
     return this._findMessage(
       msgs,
       triggerId && {
         id: triggerId,
         param: triggerParam,
         context: triggerContext,
-      }
+      },
+      { shouldCache }
     );
   }
 
   async setMessageById(id, target, force = true, action = {}) {
     await this.setState({ lastMessageId: id });
     const newMessage = this.getMessageById(id);
 
     await this._sendMessageToTarget(newMessage, target, action.data, force);
--- a/browser/components/newtab/lib/ASRouterTargeting.jsm
+++ b/browser/components/newtab/lib/ASRouterTargeting.jsm
@@ -114,16 +114,19 @@ const MOZ_JEXL_FILEPATH = "mozjexl";
 
 const { activityStreamProvider: asProvider } = NewTabUtils;
 
 const FRECENT_SITES_UPDATE_INTERVAL = 6 * 60 * 60 * 1000; // Six hours
 const FRECENT_SITES_IGNORE_BLOCKED = false;
 const FRECENT_SITES_NUM_ITEMS = 25;
 const FRECENT_SITES_MIN_FRECENCY = 100;
 
+const CACHE_EXPIRATION = 60 * 1000;
+const jexlEvaluationCache = new Map();
+
 /**
  * CachedTargetingGetter
  * @param property {string} Name of the method called on ActivityStreamProvider
  * @param options {{}?} Options object passsed to ActivityStreamProvider method
  * @param updateInterval {number?} Update interval for query. Defaults to FRECENT_SITES_UPDATE_INTERVAL
  */
 function CachedTargetingGetter(
   property,
@@ -533,31 +536,62 @@ this.ASRouterTargeting = {
         trigger.param.url &&
         new MatchPatternSet(candidateMessageTrigger.patterns).matches(
           trigger.param.url
         ))
     );
   },
 
   /**
+   * getCachedEvaluation - Return a cached jexl evaluation if available
+   *
+   * @param {string} targeting JEXL expression to lookup
+   * @returns {obj|null} Object with value result or null if not available
+   */
+  getCachedEvaluation(targeting) {
+    if (jexlEvaluationCache.has(targeting)) {
+      const { timestamp, value } = jexlEvaluationCache.get(targeting);
+      if (Date.now() - timestamp <= CACHE_EXPIRATION) {
+        return { value };
+      }
+      jexlEvaluationCache.delete(targeting);
+    }
+
+    return null;
+  },
+
+  /**
    * checkMessageTargeting - Checks is a message's targeting parameters are satisfied
    *
    * @param {*} message An AS router message
    * @param {obj} context A FilterExpression context
    * @param {func} onError A function to handle errors (takes two params; error, message)
+   * @param {boolean} shouldCache Should the JEXL evaluations be cached and reused.
    * @returns
    */
-  async checkMessageTargeting(message, context, onError) {
+  async checkMessageTargeting(message, context, onError, shouldCache) {
     // If no targeting is specified,
     if (!message.targeting) {
       return true;
     }
     let result;
     try {
+      if (shouldCache) {
+        result = this.getCachedEvaluation(message.targeting);
+        if (result) {
+          return result.value;
+        }
+      }
       result = await this.isMatch(message.targeting, context);
+      if (shouldCache) {
+        jexlEvaluationCache.set(message.targeting, {
+          timestamp: Date.now(),
+          value: result,
+        });
+      }
     } catch (error) {
       Cu.reportError(error);
       if (onError) {
         const type = error.fileName.includes(MOZ_JEXL_FILEPATH)
           ? this.ERROR_TYPES.MALFORMED_EXPRESSION
           : this.ERROR_TYPES.OTHER_ERROR;
         onError(type, error, message);
       }
@@ -572,45 +606,58 @@ this.ASRouterTargeting = {
     return sortMessagesByPriority(sortedMessages);
   },
 
   _getCombinedContext(trigger, context) {
     const triggerContext = trigger ? trigger.context : {};
     return this.combineContexts(context, triggerContext);
   },
 
-  _isMessageMatch(message, trigger, context, onError) {
+  _isMessageMatch(message, trigger, context, onError, shouldCache = false) {
     return (
       message &&
       (trigger
         ? this.isTriggerMatch(trigger, message.trigger)
         : !message.trigger) &&
       // If a trigger expression was passed to this function, the message should match it.
       // Otherwise, we should choose a message with no trigger property (i.e. a message that can show up at any time)
-      this.checkMessageTargeting(message, context, onError)
+      this.checkMessageTargeting(message, context, onError, shouldCache)
     );
   },
 
   /**
    * findMatchingMessage - Given an array of messages, returns one message
    *                       whos targeting expression evaluates to true
    *
    * @param {Array} messages An array of AS router messages
    * @param {trigger} string A trigger expression if a message for that trigger is desired
    * @param {obj|null} context A FilterExpression context. Defaults to TargetingGetters above.
    * @param {func} onError A function to handle errors (takes two params; error, message)
+   * @param {boolean} shouldCache Should the JEXL evaluations be cached and reused.
    * @returns {obj} an AS router message
    */
-  async findMatchingMessage({ messages, trigger, context, onError }) {
+  async findMatchingMessage({
+    messages,
+    trigger,
+    context,
+    onError,
+    shouldCache = false,
+  }) {
     const sortedMessages = this._getSortedMessages(messages);
     const combinedContext = this._getCombinedContext(trigger, context);
 
     for (const candidate of sortedMessages) {
       if (
-        await this._isMessageMatch(candidate, trigger, combinedContext, onError)
+        await this._isMessageMatch(
+          candidate,
+          trigger,
+          combinedContext,
+          onError,
+          shouldCache
+        )
       ) {
         return candidate;
       }
     }
     return null;
   },
 
   /**
--- a/browser/components/newtab/test/unit/asrouter/ASRouter.test.js
+++ b/browser/components/newtab/test/unit/asrouter/ASRouter.test.js
@@ -1083,21 +1083,61 @@ describe("ASRouter", () => {
       };
       await Router.setState({ messages: [message2, message1] });
       // Just return the first message provided as arg
       const stub = sandbox.stub(Router, "_findMessage");
 
       Router.handleMessageRequest(trigger);
 
       assert.calledOnce(stub);
-      assert.calledWithExactly(stub, sinon.match.array, {
-        id: trigger.triggerId,
-        param: trigger.triggerParam,
-        context: trigger.triggerContext,
-      });
+      assert.calledWithExactly(
+        stub,
+        sinon.match.array,
+        {
+          id: trigger.triggerId,
+          param: trigger.triggerParam,
+          context: trigger.triggerContext,
+        },
+        { shouldCache: false }
+      );
+    });
+    it("should cache snippets messages", async () => {
+      const trigger = {
+        triggerId: "foo",
+        triggerParam: "bar",
+        triggerContext: "context",
+      };
+      const message1 = {
+        id: "1",
+        provider: "snippets",
+        campaign: "foocampaign",
+        trigger: { id: "foo" },
+      };
+      const message2 = {
+        id: "2",
+        campaign: "foocampaign",
+        trigger: { id: "bar" },
+      };
+      await Router.setState({ messages: [message2, message1] });
+      // Just return the first message provided as arg
+      const stub = sandbox.stub(Router, "_findMessage");
+
+      Router.handleMessageRequest(trigger);
+
+      assert.calledOnce(stub);
+      assert.calledWithExactly(
+        stub,
+        sinon.match.array,
+        {
+          id: trigger.triggerId,
+          param: trigger.triggerParam,
+          context: trigger.triggerContext,
+        },
+        { shouldCache: true }
+      );
     });
     it("should filter out messages without a trigger (or different) when a triggerId is defined", async () => {
       const trigger = { triggerId: "foo" };
       const message1 = {
         id: "1",
         campaign: "foocampaign",
         trigger: { id: "foo" },
       };
--- a/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js
+++ b/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js
@@ -213,8 +213,96 @@ describe("#CachedTargetingGetter", () =>
             return "bar";
           },
         }
       );
       assert.calledOnce(global.Cu.reportError);
     });
   });
 });
+describe("ASRouterTargeting", () => {
+  let evalStub;
+  let sandbox;
+  let clock;
+  beforeEach(() => {
+    sandbox = sinon.createSandbox();
+    evalStub = sandbox.stub(global.FilterExpressions, "eval");
+    sandbox.replace(ASRouterTargeting, "Environment", {});
+    clock = sinon.useFakeTimers();
+  });
+  afterEach(() => {
+    clock.restore();
+    sandbox.restore();
+  });
+  it("should cache evaluation result", async () => {
+    evalStub.resolves(true);
+
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl1" },
+      {},
+      sandbox.stub(),
+      true
+    );
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl2" },
+      {},
+      sandbox.stub(),
+      true
+    );
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl1" },
+      {},
+      sandbox.stub(),
+      true
+    );
+
+    assert.calledTwice(evalStub);
+  });
+  it("should not cache evaluation result", async () => {
+    evalStub.resolves(true);
+
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl" },
+      {},
+      sandbox.stub(),
+      false
+    );
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl" },
+      {},
+      sandbox.stub(),
+      false
+    );
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl" },
+      {},
+      sandbox.stub(),
+      false
+    );
+
+    assert.calledThrice(evalStub);
+  });
+  it("should expire cache entries", async () => {
+    evalStub.resolves(true);
+
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl" },
+      {},
+      sandbox.stub(),
+      true
+    );
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl" },
+      {},
+      sandbox.stub(),
+      true
+    );
+    clock.tick(60 * 1000 + 1);
+    await ASRouterTargeting.checkMessageTargeting(
+      { targeting: "jexl" },
+      {},
+      sandbox.stub(),
+      true
+    );
+
+    assert.calledTwice(evalStub);
+  });
+});