Merge lp:~wgrant/launchpad/bug-736002 into lp:launchpad

[William Grant]

[r=jtv][bug=736002] Speed up +bugtarget-portlet-tags-content.

This branch fixes tags_cloud_data to to issue only two queries, hopefully minimising timeouts on +bugtarget-portlet-tags-content.

It used to display the official tags and the top 10 unofficial tags. This proves to be fairly expensive, so it will now display the official tags and the top 10 tags, some of which may duplicate the official tag set.

I've changed it to query the context's official tags, then perform a single UNION query to get the top 10 and official tag counts. Both sides of the union are done together in a single pass, and I've also avoided a slow join over Bug, letting the query complete in <4s for Ubuntu on staging.

[Jeroen T. Vermeulen (jtv)]

Looks good. On IRC you said that we don't expect anyone to have a problem with the functional change, or if they do, we can solve it in other ways later.

Another, very small thing, in lib/lp/bugs/browser/bugtask.py:

80 + tags = []
81 + for tag, count in raw_tags.iteritems():
82 + tag_dict = dict(
83 + tag=tag, count=count, url=self._getSearchURL(tag),
84 + factor=1 + (count / max_count))
85 + if tag in official_tags:
86 + tag_dict['factor'] += 0.5
87 + tags.append(tag_dict)

The "factor=1 + (count / maxcount)" is a bit misleading in terms of associativity. If you isolated the factor computation (including the "+= 0.5") in a function, this whole loop could be replaced with a list comprehension.

Jeroen

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2011-03-31 08:05:07 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2011-04-12 08:33:41 +0000
@@ -1373,63 +1373,38 @@
         # Use path_only here to reduce the size of the rendered page.
         return "+bugs?field.tag=%s" % urllib.quote(tag)
 
-    def getUsedBugTagsWithURLs(self):
-        """Return the bug tags and their search URLs."""
-        bug_tag_counts = self.context.getUsedBugTagsWithOpenCounts(self.user)
-        return [
-            {'tag': tag, 'count': count, 'url': self._getSearchURL(tag)}
-            for tag, count in bug_tag_counts]
-
-    @property
-    def official_tags(self):
-        """Get the official tags to diplay."""
-        official_tags = set(self.context.official_bug_tags)
-        tags = [tag for tag in self.getUsedBugTagsWithURLs()
-                if tag['tag'] in official_tags]
-        used_tags = set(tag['tag'] for tag in tags)
-        tags.sort(key=itemgetter('count'), reverse=True)
-        for tag in sorted(official_tags - used_tags):
-            tags.append(
-                {'tag': tag, 'count': 0, 'url': self._getSearchURL(tag)})
-        return tags
-
-    @property
-    def other_tags(self):
-        """Get the unofficial tags to diplay."""
-        official_tags = set(self.context.official_bug_tags)
-        tags = [tag for tag in self.getUsedBugTagsWithURLs()
-                if tag['tag'] not in official_tags]
-        tags.sort(key=itemgetter('count'), reverse=True)
-        return tags[:10]
+    def _calculateFactor(self, tag, count, max_count, official_tags):
+        bonus = 1.5 if tag in official_tags else 1
+        return (count / max_count) + bonus
 
     @property
     def tags_cloud_data(self):
         """The data for rendering a tags cloud"""
-        official_tags = list(self.context.official_bug_tags)
-        tags = self.getUsedBugTagsWithURLs()
-        other_tags = [tag for tag in tags if tag['tag'] not in official_tags]
-        popular_tags = [tag['tag'] for tag in sorted(
-            other_tags, key=itemgetter('count'), reverse=True)[:10]]
-        tags = [
-            tag for tag in tags
-            if tag['tag'] in official_tags + popular_tags]
-        all_tag_dicts = [tag['tag'] for tag in tags]
-        for official_tag in official_tags:
-            if official_tag not in all_tag_dicts:
-                tags.append({
-                    'tag': official_tag,
-                    'count': 0,
-                    'url': "+bugs?field.tag=%s" % urllib.quote(official_tag)})
-        max_count = float(max([1] + [tag['count'] for tag in tags]))
-        for tag in tags:
-            if tag['tag'] in official_tags:
-                if tag['count'] == 0:
-                    tag['factor'] = 1.5
-                else:
-                    tag['factor'] = 1.5 + (tag['count'] / max_count)
-            else:
-                tag['factor'] = 1 + (tag['count'] / max_count)
-        return sorted(tags, key=itemgetter('tag'))
+        official_tags = self.context.official_bug_tags
+
+        # Construct a dict of official and top 10 tags.
+        # getUsedBugTagsWithOpenCounts is expensive, so do the union in
+        # SQL. Also preseed with 0 for all the official tags, as gUBTWOC
+        # won't return unused ones.
+        top_ten = removeSecurityProxy(
+            self.context.getUsedBugTagsWithOpenCounts(self.user)[:10])
+        official = removeSecurityProxy(
+            self.context.getUsedBugTagsWithOpenCounts(
+                self.user, official_tags))
+        tags = dict((tag, 0) for tag in official_tags)
+        tags.update(dict(top_ten.union(official)))
+
+        max_count = float(max([1] + tags.values()))
+
+        return sorted(
+            [dict(
+                tag=tag,
+                factor=self._calculateFactor(
+                    tag, count, max_count, official_tags),
+                url=self._getSearchURL(tag),
+                )
+            for (tag, count) in tags.iteritems()],
+            key=itemgetter('tag'))
 
     @property
     def show_manage_tags_link(self):

=== modified file 'lib/lp/bugs/doc/bug-tags.txt'
--- lib/lp/bugs/doc/bug-tags.txt	2011-01-11 09:40:05 +0000
+++ lib/lp/bugs/doc/bug-tags.txt	2011-04-12 08:33:41 +0000
@@ -332,27 +332,35 @@
 We can also get all the used tags, together with the number of open
 bugs each tag has. Only tags having open bugs are returned.
 
-    >>> firefox.getUsedBugTagsWithOpenCounts(None)
-    [(u'doc', 1L), (u'sco', 1L), (u'svg', 1L)]
-
-    >>> mozilla.getUsedBugTagsWithOpenCounts(None)
-    [(u'doc', 1L), (u'sco', 1L), (u'svg', 1L)]
-
-    >>> ubuntu.getUsedBugTagsWithOpenCounts(None)
+    >>> list(firefox.getUsedBugTagsWithOpenCounts(None))
+    [(u'doc', 1L), (u'sco', 1L), (u'svg', 1L)]
+
+    >>> list(mozilla.getUsedBugTagsWithOpenCounts(None))
+    [(u'doc', 1L), (u'sco', 1L), (u'svg', 1L)]
+
+    >>> list(ubuntu.getUsedBugTagsWithOpenCounts(None))
     [(u'crash', 2L), (u'dataloss', 1L), (u'pebcak', 1L),
      (u'sco', 1L), (u'svg', 1L)]
 
+We can even ask for the counts for a particular set of tags.
+
+    >>> list(ubuntu.getUsedBugTagsWithOpenCounts(
+    ...     None, wanted_tags=['pebcak', 'svg', 'fake']))
+    [(u'pebcak', 1L), (u'svg', 1L)]
+    >>> list(ubuntu.getUsedBugTagsWithOpenCounts(None, wanted_tags=[]))
+    []
+
 Source packages are a bit special, they return all the tags that are
 used in the whole distribution, while the bug count includes only bugs
 in the specific package.
 
-    >>> ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(None)
+    >>> list(ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(None))
     [(u'crash', 1L)]
 
-    >>> debian_woody.getUsedBugTagsWithOpenCounts(None)
+    >>> list(debian_woody.getUsedBugTagsWithOpenCounts(None))
     [(u'dataloss', 1L), (u'layout-test', 1L), (u'pebcak', 1L)]
 
-    >>> debian_woody_firefox.getUsedBugTagsWithOpenCounts(None)
+    >>> list(debian_woody_firefox.getUsedBugTagsWithOpenCounts(None))
     [(u'dataloss', 1L), (u'layout-test', 1L), (u'pebcak', 1L)]
 
 Only bugs that the supplied user has access to will be counted:
@@ -362,13 +370,13 @@
     True
     >>> flush_database_updates()
 
-    >>> ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(None)
+    >>> list(ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(None))
     []
 
     >>> sample_person = getUtility(ILaunchBag).user
     >>> bug_nine.isSubscribed(sample_person)
     True
-    >>> ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(sample_person)
+    >>> list(ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(sample_person))
     [(u'crash', 1L)]
 
 When context doesn't have any tags getUsedBugTags() returns a empty list.

=== modified file 'lib/lp/bugs/interfaces/bugtarget.py'
--- lib/lp/bugs/interfaces/bugtarget.py	2011-03-28 20:54:50 +0000
+++ lib/lp/bugs/interfaces/bugtarget.py	2011-04-12 08:33:41 +0000
@@ -403,13 +403,15 @@
     def getUsedBugTags():
         """Return the tags used by the context as a sorted list of strings."""
 
-    def getUsedBugTagsWithOpenCounts(user):
+    def getUsedBugTagsWithOpenCounts(user, wanted_tags=None):
         """Return name and bug count of tags having open bugs.
 
         It returns a list of tuples contining the tag name, and the
         number of open bugs having that tag. Only the bugs that the user
         has permission to see are counted, and only tags having open
         bugs will be returned.
+
+        If wanted_tags is specified, only those tags will be returned.
         """
 
     def _getOfficialTagClause():

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-04-07 16:23:24 +0000
+++ lib/lp/bugs/model/bug.py	2011-04-12 08:33:41 +0000
@@ -50,6 +50,9 @@
 from storm.expr import (
     And,
     Count,
+    Desc,
+    Exists,
+    Join,
     LeftJoin,
     Max,
     Not,
@@ -211,6 +214,29 @@
             %(condition)s GROUP BY BugTag.tag ORDER BY BugTag.tag"""
 
 
+def snapshot_bug_params(bug_params):
+    """Return a snapshot of a `CreateBugParams` object."""
+    return Snapshot(
+        bug_params, names=[
+            "owner", "title", "comment", "description", "msg",
+            "datecreated", "security_related", "private",
+            "distribution", "sourcepackagename", "binarypackagename",
+            "product", "status", "subscribers", "tags",
+            "subscribe_owner", "filed_by"])
+
+
+class BugTag(SQLBase):
+    """A tag belonging to a bug."""
+
+    bug = ForeignKey(dbName='bug', foreignKey='Bug', notNull=True)
+    tag = StringCol(notNull=True)
+
+
+# We need to always use the same Count instance or the
+# get_bug_tags_open_count is not UNIONable.
+tag_count_columns = (BugTag.tag, Count())
+
+
 def get_bug_tags(context_clause):
     """Return all the bug tags as a list of strings.
 
@@ -230,59 +256,39 @@
     return shortlist([row[0] for row in cur.fetchall()])
 
 
-def get_bug_tags_open_count(context_condition, user):
+def get_bug_tags_open_count(context_condition, user, wanted_tags=None):
     """Return all the used bug tags with their open bug count.
 
     :param context_condition: A Storm SQL expression, limiting the
         used tags to a specific context. Only the BugTask table may be
         used to choose the context.
     :param user: The user performing the search.
+    :param wanted_tags: A set of tags within which to restrict the search.
 
     :return: A list of tuples, (tag name, open bug count).
     """
-    open_statuses_condition = BugTask.status.is_in(
-        UNRESOLVED_BUGTASK_STATUSES)
-    columns = [
-        BugTag.tag,
-        Count(),
-        ]
-    tables = [
+    tables = (
         BugTag,
-        LeftJoin(Bug, Bug.id == BugTag.bugID),
-        LeftJoin(
-            BugTask,
-            And(BugTask.bugID == Bug.id, open_statuses_condition)),
-        ]
+        Join(BugTask, BugTask.bugID == BugTag.bugID),
+        )
     where_conditions = [
-        open_statuses_condition,
+        BugTask.status.is_in(UNRESOLVED_BUGTASK_STATUSES),
         context_condition,
         ]
+    if wanted_tags is not None:
+        where_conditions.append(BugTag.tag.is_in(wanted_tags))
     privacy_filter = get_bug_privacy_filter(user)
     if privacy_filter:
-        where_conditions.append(SQLRaw(privacy_filter))
+        # The EXISTS sub-select avoids a join against Bug, improving
+        # performance significantly.
+        where_conditions.append(
+            Exists(Select(
+                columns=[True], tables=[Bug],
+                where=And(Bug.id == BugTag.bugID, SQLRaw(privacy_filter)))))
     store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-    result = store.execute(Select(
-        columns=columns, where=And(*where_conditions), tables=tables,
-        group_by=BugTag.tag, order_by=BugTag.tag))
-    return shortlist([(row[0], row[1]) for row in result.get_all()])
-
-
-def snapshot_bug_params(bug_params):
-    """Return a snapshot of a `CreateBugParams` object."""
-    return Snapshot(
-        bug_params, names=[
-            "owner", "title", "comment", "description", "msg",
-            "datecreated", "security_related", "private",
-            "distribution", "sourcepackagename", "binarypackagename",
-            "product", "status", "subscribers", "tags",
-            "subscribe_owner", "filed_by"])
-
-
-class BugTag(SQLBase):
-    """A tag belonging to a bug."""
-
-    bug = ForeignKey(dbName='bug', foreignKey='Bug', notNull=True)
-    tag = StringCol(notNull=True)
+    return store.using(*tables).find(
+        tag_count_columns, *where_conditions).group_by(BugTag.tag).order_by(
+            Desc(Count()), BugTag.tag)
 
 
 class BugBecameQuestionEvent:

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2011-04-08 15:44:02 +0000
+++ lib/lp/registry/model/distribution.py	2011-04-12 08:33:41 +0000
@@ -649,9 +649,10 @@
         """See `IBugTarget`."""
         return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
 
-    def getUsedBugTagsWithOpenCounts(self, user):
+    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
         """See `IBugTarget`."""
-        return get_bug_tags_open_count(BugTask.distribution == self, user)
+        return get_bug_tags_open_count(
+            BugTask.distribution == self, user, wanted_tags=wanted_tags)
 
     def getMirrorByName(self, name):
         """See `IDistribution`."""

=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
--- lib/lp/registry/model/distributionsourcepackage.py	2011-03-23 16:28:51 +0000
+++ lib/lp/registry/model/distributionsourcepackage.py	2011-04-12 08:33:41 +0000
@@ -488,12 +488,12 @@
         """See `IBugTarget`."""
         return self.distribution.getUsedBugTags()
 
-    def getUsedBugTagsWithOpenCounts(self, user):
+    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
         """See `IBugTarget`."""
         return get_bug_tags_open_count(
             And(BugTask.distribution == self.distribution,
                 BugTask.sourcepackagename == self.sourcepackagename),
-            user)
+            user, wanted_tags=wanted_tags)
 
     def _getOfficialTagClause(self):
         return self.distribution._getOfficialTagClause()

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-04-08 15:44:02 +0000
+++ lib/lp/registry/model/distroseries.py	2011-04-12 08:33:41 +0000
@@ -833,9 +833,10 @@
         """See `IHasBugs`."""
         return get_bug_tags("BugTask.distroseries = %s" % sqlvalues(self))
 
-    def getUsedBugTagsWithOpenCounts(self, user):
+    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
         """See `IHasBugs`."""
-        return get_bug_tags_open_count(BugTask.distroseries == self, user)
+        return get_bug_tags_open_count(
+            BugTask.distroseries == self, user, wanted_tags=wanted_tags)
 
     @property
     def has_any_specifications(self):

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2011-03-28 20:54:50 +0000
+++ lib/lp/registry/model/product.py	2011-04-12 08:33:41 +0000
@@ -800,9 +800,10 @@
         """See `IBugTarget`."""
         return get_bug_tags("BugTask.product = %s" % sqlvalues(self))
 
-    def getUsedBugTagsWithOpenCounts(self, user):
+    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
         """See `IBugTarget`."""
-        return get_bug_tags_open_count(BugTask.product == self, user)
+        return get_bug_tags_open_count(
+            BugTask.product == self, user, wanted_tags=wanted_tags)
 
     series = SQLMultipleJoin('ProductSeries', joinColumn='product',
         orderBy='name')

=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py	2011-03-28 20:54:50 +0000
+++ lib/lp/registry/model/productseries.py	2011-04-12 08:33:41 +0000
@@ -433,9 +433,10 @@
         """See IBugTarget."""
         return get_bug_tags("BugTask.productseries = %s" % sqlvalues(self))
 
-    def getUsedBugTagsWithOpenCounts(self, user):
+    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
         """See IBugTarget."""
-        return get_bug_tags_open_count(BugTask.productseries == self, user)
+        return get_bug_tags_open_count(
+            BugTask.productseries == self, user, wanted_tags=wanted_tags)
 
     def createBug(self, bug_params):
         """See IBugTarget."""

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2011-01-21 08:30:55 +0000
+++ lib/lp/registry/model/projectgroup.py	2011-04-12 08:33:41 +0000
@@ -343,13 +343,14 @@
         return get_bug_tags(
             "BugTask.product IN (%s)" % ",".join(product_ids))
 
-    def getUsedBugTagsWithOpenCounts(self, user):
+    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
         """See `IHasBugs`."""
         if not self.products:
             return []
         product_ids = [product.id for product in self.products]
         return get_bug_tags_open_count(
-            BugTask.productID.is_in(product_ids), user)
+            BugTask.productID.is_in(product_ids), user,
+            wanted_tags=wanted_tags)
 
     def _getBugTaskContextClause(self):
         """See `HasBugsBase`."""

=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py	2011-04-07 19:08:45 +0000
+++ lib/lp/registry/model/sourcepackage.py	2011-04-12 08:33:41 +0000
@@ -495,12 +495,12 @@
         """See `IBugTarget`."""
         return self.distroseries.getUsedBugTags()
 
-    def getUsedBugTagsWithOpenCounts(self, user):
+    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
         """See `IBugTarget`."""
         return get_bug_tags_open_count(
             And(BugTask.distroseries == self.distroseries,
                 BugTask.sourcepackagename == self.sourcepackagename),
-            user)
+            user, wanted_tags=wanted_tags)
 
     @property
     def max_bug_heat(self):