bug 1111032: Implement rule caching with a few parts of the update qu…

…ery as the key (#97). r=nthomas,rail,mostlygeek

auslib/db.py

@@ -787,32 +787,47 @@ def getRulesMatchingQuery(self, updateQuery, fallbackChannel, transaction=None):
         """Returns all of the rules that match the given update query.
            For cases where a particular updateQuery channel has no
            fallback, fallbackChannel should match the channel from the query."""
-        where = [
-            ((self.product == updateQuery['product']) | (self.product == null())) &
-            ((self.buildTarget == updateQuery['buildTarget']) | (self.buildTarget == null())) &
-            ((self.headerArchitecture == updateQuery['headerArchitecture']) | (self.headerArchitecture == null()))
-        ]
-        # Query version 2 doesn't have distribution information, and to keep
-        # us maximally flexible, we won't match any rules that have
-        # distribution update set.
-        if updateQuery['queryVersion'] == 2:
-            where.extend([(self.distribution == null()) & (self.distVersion == null())])
-        # Only query versions 3 and 4 have distribution information, so we
-        # need to consider it.
-        if updateQuery['queryVersion'] in (3, 4):
-            where.extend([
-                ((self.distribution == updateQuery['distribution']) | (self.distribution == null())) &
-                ((self.distVersion == updateQuery['distVersion']) | (self.distVersion == null()))
-            ])
-        if not updateQuery['force']:
-            where.append(self.backgroundRate > 0)
-        rules = self.select(where=where, transaction=transaction)
-        self.log.debug("where: %s" % where)
+
+        def getRawMatches():
+            where = [
+                ((self.product == updateQuery['product']) | (self.product == null())) &
+                ((self.buildTarget == updateQuery['buildTarget']) | (self.buildTarget == null())) &
+                ((self.headerArchitecture == updateQuery['headerArchitecture']) | (self.headerArchitecture == null()))
+            ]
+            # Query version 2 doesn't have distribution information, and to keep
+            # us maximally flexible, we won't match any rules that have
+            # distribution update set.
+            if updateQuery['queryVersion'] == 2:
+                where.extend([(self.distribution == null()) & (self.distVersion == null())])
+            # Only query versions 3 and 4 have distribution information, so we
+            # need to consider it.
+            if updateQuery['queryVersion'] in (3, 4):
+                where.extend([
+                    ((self.distribution == updateQuery['distribution']) | (self.distribution == null())) &
+                    ((self.distVersion == updateQuery['distVersion']) | (self.distVersion == null()))
+                ])
+            if not updateQuery['force']:
+                where.append(self.backgroundRate > 0)
+
+            self.log.debug("where: %s" % where)
+            return self.select(where=where, transaction=transaction)
+
+        # This cache key is constructed from all parts of the updateQuery that
+        # are used in the select() to get the "raw" rule matches. For the most
+        # part, product and buildTarget will be the only applicable ones which
+        # means we should get very high cache hit rates, as there's not a ton
+        # of variability of possible combinations for those.
+        cache_key = "%s:%s:%s:%s:%s:%s" % \
+            (updateQuery["product"], updateQuery["buildTarget"], updateQuery["headerArchitecture"],
+             updateQuery.get("distribution"), updateQuery.get("distVersion"), updateQuery["force"])
+        rules = cache.get("rules", cache_key, getRawMatches)
+
         self.log.debug("Raw matches:")
 
         matchingRules = []
         for rule in rules:
             self.log.debug(rule)
+
             # Resolve special means for channel, version, and buildID - dropping
             # rules that don't match after resolution.
             if not self._channelMatchesRule(rule['channel'], updateQuery['channel'], fallbackChannel):

auslib/test/test_db.py

@@ -1177,6 +1177,148 @@ def testProceedIfNotReadOnly(self):
         self.assertRaises(ReadOnlyError, self.releases._proceedIfNotReadOnly, 'a')
 
 
+class TestRulesCaching(unittest.TestCase, MemoryDatabaseMixin, RulesTestMixin):
+
+    def setUp(self):
+        MemoryDatabaseMixin.setUp(self)
+        cache.reset()
+        cache.make_copies = True
+        cache.make_cache("rules", 20, 4)
+        self.db = AUSDatabase(self.dburi)
+        self.db.create()
+        self.rules = self.db.rules
+        self.rules.t.insert().execute(rule_id=1, priority=100, version='3.5', buildTarget='d', backgroundRate=100, mapping='c', update_type='z', data_version=1)
+        self.rules.t.insert().execute(rule_id=2, priority=100, version='3.3', buildTarget='d', backgroundRate=100, mapping='b', update_type='z', data_version=1)
+        self.rules.t.insert().execute(rule_id=3, priority=100, version='3.5', buildTarget='a', backgroundRate=100, mapping='a', update_type='z', data_version=1)
+
+    def tearDown(self):
+        cache.reset()
+
+    def _checkCacheStats(self, cache, lookups, hits, misses):
+        self.assertEquals(cache.lookups, lookups)
+        self.assertEquals(cache.hits, hits)
+        self.assertEquals(cache.misses, misses)
+
+    def testGetRulesMatchingQueryCacheKeysAreCorrect(self):
+        """Try a few different queries to make sure cache keys are constructed correctly."""
+        self.rules.getRulesMatchingQuery(
+            dict(product='', version='3.5', channel='',
+                 buildTarget='a', buildID='', locale='', osVersion='',
+                 distribution='', distVersion='', headerArchitecture='',
+                 force=False, queryVersion=3),
+            fallbackChannel=''
+        )
+        self.rules.getRulesMatchingQuery(
+            dict(product='c', version='3.5', channel='',
+                 buildTarget='a', buildID='', locale='', osVersion='',
+                 distribution='', distVersion='', headerArchitecture='',
+                 force=False, queryVersion=3),
+            fallbackChannel=''
+        )
+        self.rules.getRulesMatchingQuery(
+            dict(product='b', version='3.5', channel='',
+                 buildTarget='e', buildID='', locale='', osVersion='',
+                 distribution='', distVersion='', headerArchitecture='',
+                 force=True, queryVersion=3),
+            fallbackChannel=''
+        )
+        expected = set([
+            ":a::::False",
+            "c:a::::False",
+            "b:e::::True",
+        ])
+        self.assertEquals(set(cache.caches["rules"].data.keys()), expected)
+
+    def testGetRulesMatchingQueryUsesCachedRules(self):
+        """Ensure that getRulesMatchingQuery properly uses the rules cache"""
+        with mock.patch("time.time") as t:
+            t.return_value = 0
+            for i in range(5):
+                rules = self.rules.getRulesMatchingQuery(
+                    dict(product='', version='3.5', channel='',
+                         buildTarget='a', buildID='', locale='', osVersion='',
+                         distribution='', distVersion='', headerArchitecture='',
+                         force=False, queryVersion=3,
+                         ),
+                    fallbackChannel=''
+                )
+                rules = self._stripNullColumns(rules)
+                expected = [dict(rule_id=3, priority=100, backgroundRate=100, version='3.5', buildTarget='a', mapping='a', update_type='z', data_version=1)]
+                self.assertEquals(rules, expected)
+
+                t.return_value += 1
+
+            self._checkCacheStats(cache.caches["rules"], 5, 3, 2)
+
+    def testGetRulesMatchingQueryRefreshesAfterExpiry(self):
+        """Ensure that getRulesMatchingQuery picks up changes to the rules table after expiry"""
+        with mock.patch("time.time") as t:
+            t.return_value = 0
+            for i in range(3):
+                rules = self.rules.getRulesMatchingQuery(
+                    dict(product='', version='3.5', channel='',
+                         buildTarget='a', buildID='', locale='', osVersion='',
+                         distribution='', distVersion='', headerArchitecture='',
+                         force=False, queryVersion=3),
+                    fallbackChannel=''
+                )
+                rules = self._stripNullColumns(rules)
+                expected = [dict(rule_id=3, priority=100, backgroundRate=100, version='3.5', buildTarget='a', mapping='a', update_type='z', data_version=1)]
+                self.assertEquals(rules, expected)
+
+                t.return_value += 1
+
+            self.rules.t.update(values=dict(mapping="b")).where(self.rules.rule_id == 3).execute()
+
+            rules = self.rules.getRulesMatchingQuery(
+                dict(product='', version='3.5', channel='',
+                     buildTarget='a', buildID='', locale='', osVersion='',
+                     distribution='', distVersion='', headerArchitecture='',
+                     force=False, queryVersion=3),
+                fallbackChannel=''
+            )
+            rules = self._stripNullColumns(rules)
+            expected = [dict(rule_id=3, priority=100, backgroundRate=100, version='3.5', buildTarget='a', mapping='a', update_type='z', data_version=1)]
+            self.assertEquals(rules, expected)
+
+            t.return_value += 1
+
+            for i in range(2):
+                rules = self.rules.getRulesMatchingQuery(
+                    dict(product='', version='3.5', channel='',
+                         buildTarget='a', buildID='', locale='', osVersion='',
+                         distribution='', distVersion='', headerArchitecture='',
+                         force=False, queryVersion=3),
+                    fallbackChannel=''
+                )
+                rules = self._stripNullColumns(rules)
+                expected = [dict(rule_id=3, priority=100, backgroundRate=100, version='3.5', buildTarget='a', mapping='b', update_type='z', data_version=1)]
+                self.assertEquals(rules, expected)
+
+                t.return_value += 1
+
+            self._checkCacheStats(cache.caches["rules"], 6, 4, 2)
+
+    def testGetRulesMatchingQueryWithFunkyQuery(self):
+        """Ensure that an unsubstituted query caches properly."""
+        with mock.patch("time.time") as t:
+            t.return_value = 0
+            for i in range(5):
+                rules = self.rules.getRulesMatchingQuery(
+                    dict(product="%PRODUCT%", version="%VERSION%", channel="%CHANNEL%", buildTarget="%BUILD_TARGET%",
+                         buildID="%BUILDID%", locale="%LOCALE%", osVersion="%OS_VERSION%",
+                         distribution="%DISTRIBUTION%", distVersion="%DIST_VERSION%", headerArchitecture="",
+                         force=False, queryVersion=3),
+                    fallbackChannel=''
+                )
+                rules = self._stripNullColumns(rules)
+                self.assertEquals(rules, [])
+
+                t.return_value += 1
+
+            self._checkCacheStats(cache.caches["rules"], 5, 3, 2)
+
+
 class TestBlobCaching(unittest.TestCase, MemoryDatabaseMixin):
 
     def setUp(self):

auslib/util/cache.py

@@ -54,7 +54,10 @@ def get(self, name, key, value_getter=None):
                 return None
 
         value = self.caches[name].get(key)
-        if not value and callable(value_getter):
+        # "if value is None" is important here (instead of "if not value")
+        # because it allows us to cache results of potentially expensive
+        # calls that may end up returning nothing.
+        if value is None and callable(value_getter):
             value = value_getter()
             self.put(name, key, value)
 

uwsgi/public.wsgi

@@ -37,6 +37,10 @@ cache.make_cache("blob", 500, 3600)
 cache.make_cache("blob_schema", 50, 24 * 60 * 60)
 cache.make_cache("blob_version", 500, 60)
 
+# 500 is probably a bit oversized for the rules cache, but the items are so
+# small there sholudn't be any negative effect.
+cache.make_cache("rules", 500, 30)
+
 dbo.setDb(os.environ["DBURI"])
 dbo.setDomainWhitelist(DOMAIN_WHITELIST)
 application.config["WHITELISTED_DOMAINS"] = DOMAIN_WHITELIST