Merge lp:~lifeless/launchpad/bug-793809 into lp:launchpad

[Robert Collins]

[r=wgrant][bug=793809] Use BugSummary to do tag portlet calculations

Per the linked bug generating bug tag portlets is extraordinarily slow. This is meant to be addressed by querying bugsummary, which this branch implements.

I have changed the interface to match our needs allowing a faster query (the with clause on teams for visibility).

Other than that I can see a path to reduce the duplicate code that we use here, but its unrelated to fixing this bug, so I've focused on getting the bug fixed ;)

[William Grant (wgrant)]

21 + tags = self.context.getUsedBugTagsWithOpenCounts(self.user, 10,
22 + official_tags)

Nice cleanup. But bad indentation.

33 + official_tags = self.context.official_bug_tags

Sure this doesn't need to be materialised?

131 + :param tag_limit: The number of tags to return (excludes those found by
132 + matching include_tags). If 0 then all tags are returned. If
133 + non-zero then the most frequently used tags are returned.

0? Why not None?

172 +def get_bug_tags_open_count(context_condition, user, tag_limit=0,
173 + include_tags=None):

Bad indentation.

200 + store = store.with_(SQL(
201 + "teams AS ("
202 + "SELECT team from TeamParticipation WHERE person=%s)" % user.id))

String formatting in SQL: Australia says no.

Also, calling that "store" is a bit too much of a lie for my tastes.

228 + BugSummary.viewed_by_id.is_in(SQL("SELECT team FROM teams"))

What's the benefit of the WITH clause here? I doubt there is any performance gain, and including the full thing here is shorter and clearer.

review: Approve (code)

[Robert Collins (lifeless)]

> 21 + tags = self.context.getUsedBugTagsWithOpenCounts(self.user,
> 10,
> 22 + official_tags)
>
> Nice cleanup. But bad indentation.

How so ?

> 33 + official_tags = self.context.official_bug_tags
>
> Sure this doesn't need to be materialised?

The core function returns a list.

> 131 + :param tag_limit: The number of tags to return (excludes
> those found by
> 132 + matching include_tags). If 0 then all tags are returned.
> If
> 133 + non-zero then the most frequently used tags are returned.
>
> 0? Why not None?

Shrug; can change it to that, but 0 would be noddy. Using 0 as the flag avoids bad behaviour if a limit of 0 is passed in.

> 172 +def get_bug_tags_open_count(context_condition, user, tag_limit=0,
> 173 + include_tags=None):
>
> Bad indentation.

How so ?

> 200 + store = store.with_(SQL(
> 201 + "teams AS ("
> 202 + "SELECT team from TeamParticipation WHERE person=%s)" %
> user.id))
>
> String formatting in SQL: Australia says no.
>
> Also, calling that "store" is a bit too much of a lie for my tastes.

It meets the core contract and is better understood than 'resultset_factory'.

> 228 + BugSummary.viewed_by_id.is_in(SQL("SELECT team FROM
> teams"))
>
> What's the benefit of the WITH clause here? I doubt there is any performance
> gain, and including the full thing here is shorter and clearer.

Its faster. 150ms difference when I was testing.

[William Grant (wgrant)]

Thanks!

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2011-06-06 06:44:23 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2011-06-07 05:39:33 +0000
@@ -1405,19 +1405,8 @@
     def tags_cloud_data(self):
         """The data for rendering a tags cloud"""
         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)))
-
+        tags = self.context.getUsedBugTagsWithOpenCounts(
+            self.user, 10, official_tags)
         max_count = float(max([1] + tags.values()))
 
         return sorted(
@@ -1462,8 +1451,8 @@
     @property
     def tags_js_data(self):
         """Return the JSON representation of the bug tags."""
-        used_tags = dict(self.context.getUsedBugTagsWithOpenCounts(self.user))
-        official_tags = list(self.context.official_bug_tags)
+        used_tags = self.context.getUsedBugTagsWithOpenCounts(self.user)
+        official_tags = self.context.official_bug_tags
         return """<script type="text/javascript">
                       var used_bug_tags = %s;
                       var official_bug_tags = %s;

=== modified file 'lib/lp/bugs/doc/bug-tags.txt'
--- lib/lp/bugs/doc/bug-tags.txt	2011-04-12 06:40:51 +0000
+++ lib/lp/bugs/doc/bug-tags.txt	2011-06-07 05:39:33 +0000
@@ -332,35 +332,34 @@
 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.
 
-    >>> 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))
+    >>> sorted(firefox.getUsedBugTagsWithOpenCounts(None).items())
+    [(u'doc', 1L), (u'sco', 1L), (u'svg', 1L)]
+
+    >>> sorted(mozilla.getUsedBugTagsWithOpenCounts(None).items())
+    [(u'doc', 1L), (u'sco', 1L), (u'svg', 1L)]
+
+    >>> sorted(ubuntu.getUsedBugTagsWithOpenCounts(None).items())
     [(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.
+We can require that some tags be included in the output even when limiting the
+results.
 
-    >>> list(ubuntu.getUsedBugTagsWithOpenCounts(
-    ...     None, wanted_tags=['pebcak', 'svg', 'fake']))
-    [(u'pebcak', 1L), (u'svg', 1L)]
-    >>> list(ubuntu.getUsedBugTagsWithOpenCounts(None, wanted_tags=[]))
-    []
+    >>> sorted(ubuntu.getUsedBugTagsWithOpenCounts(None,
+    ...     tag_limit=1, include_tags=[u'pebcak', u'svg', u'fake']).items())
+    [(u'crash', 2L), (u'fake', 0), (u'pebcak', 1L), (u'svg', 1L)]
 
 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.
 
-    >>> list(ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(None))
-    [(u'crash', 1L)]
+    >>> ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(None)
+    {u'crash': 1L}
 
-    >>> list(debian_woody.getUsedBugTagsWithOpenCounts(None))
+    >>> sorted(debian_woody.getUsedBugTagsWithOpenCounts(None).items())
     [(u'dataloss', 1L), (u'layout-test', 1L), (u'pebcak', 1L)]
 
-    >>> list(debian_woody_firefox.getUsedBugTagsWithOpenCounts(None))
+    >>> sorted(debian_woody_firefox.getUsedBugTagsWithOpenCounts(None).items())
     [(u'dataloss', 1L), (u'layout-test', 1L), (u'pebcak', 1L)]
 
 Only bugs that the supplied user has access to will be counted:
@@ -370,14 +369,14 @@
     True
     >>> flush_database_updates()
 
-    >>> list(ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(None))
-    []
+    >>> ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(None)
+    {}
 
     >>> sample_person = getUtility(ILaunchBag).user
     >>> bug_nine.isSubscribed(sample_person)
     True
-    >>> list(ubuntu_thunderbird.getUsedBugTagsWithOpenCounts(sample_person))
-    [(u'crash', 1L)]
+    >>> 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-04-12 06:21:39 +0000
+++ lib/lp/bugs/interfaces/bugtarget.py	2011-06-07 05:39:33 +0000
@@ -403,15 +403,17 @@
     def getUsedBugTags():
         """Return the tags used by the context as a sorted list of strings."""
 
-    def getUsedBugTagsWithOpenCounts(user, wanted_tags=None):
+    def getUsedBugTagsWithOpenCounts(user, tag_limit=0, include_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.
+        :param user: The user who wants the report.
+        :param tag_limit: The number of tags to return (excludes those found by
+            matching include_tags). If 0 then all tags are returned. If
+            non-zero then the most frequently used tags are returned.
+        :param include_tags: A list of string tags to return irrespective of
+            usage. Tags in this list that have no open bugs are returned with a
+            count of 0. May be None if there are tags to require inclusion of.
+        :return: A dict from tag -> count.
         """
 
     def _getOfficialTagClause():

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-06-03 10:38:25 +0000
+++ lib/lp/bugs/model/bug.py	2011-06-07 05:39:33 +0000
@@ -63,6 +63,7 @@
     Select,
     SQL,
     SQLRaw,
+    Sum,
     Union,
     )
 from storm.info import ClassAlias
@@ -242,11 +243,6 @@
     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.
 
@@ -266,39 +262,56 @@
     return shortlist([row[0] for row in cur.fetchall()])
 
 
-def get_bug_tags_open_count(context_condition, user, wanted_tags=None):
-    """Return all the used bug tags with their open bug count.
-
+def get_bug_tags_open_count(context_condition, user, tag_limit=0,
+    include_tags=None):
+    """Worker for IBugTarget.getUsedBugTagsWithOpenCounts.
+
+    See `IBugTarget` for details.
+
+    The only change is that this function takes a SQL expression for limiting
+    the found tags.
     :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).
     """
-    tables = (
-        BugTag,
-        Join(BugTask, BugTask.bugID == BugTag.bugID),
-        )
+    # Circular fail.
+    from lp.bugs.model.bugsummary import BugSummary
+    tags = {}
+    if include_tags:
+        tags = dict((tag, 0) for tag in include_tags)
+    store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+    admin_team = getUtility(ILaunchpadCelebrities).admin
+    if user is not None and not user.inTeam(admin_team):
+        store = store.with_(SQL(
+            "teams AS ("
+            "SELECT team from TeamParticipation WHERE person=?)", (user.id,)))
     where_conditions = [
-        BugTask.status.is_in(UNRESOLVED_BUGTASK_STATUSES),
+        BugSummary.status.is_in(UNRESOLVED_BUGTASK_STATUSES),
+        BugSummary.tag != None,
         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:
-        # The EXISTS sub-select avoids a join against Bug, improving
-        # performance significantly.
+    if user is None:
+        where_conditions.append(BugSummary.viewed_by_id == None)
+    elif not user.inTeam(admin_team):
         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)
-    return store.using(*tables).find(
-        tag_count_columns, *where_conditions).group_by(BugTag.tag).order_by(
-            Desc(Count()), BugTag.tag)
+            Or(
+                BugSummary.viewed_by_id == None,
+                BugSummary.viewed_by_id.is_in(SQL("SELECT team FROM teams"))
+                ))
+    tag_count_columns = (BugSummary.tag, Sum(BugSummary.count))
+    # Always query for used
+    def _query(*args):
+        return store.find(tag_count_columns, *(where_conditions + list(args))
+            ).group_by(BugSummary.tag).order_by(
+            Desc(Sum(BugSummary.count)), BugSummary.tag)
+    used = _query()
+    if tag_limit:
+        used = used[:tag_limit]
+    if include_tags:
+        # Union in a query for just include_tags.
+        used = used.union(_query(BugSummary.tag.is_in(include_tags)))
+    tags.update(dict(used))
+    return tags
 
 
 class BugBecameQuestionEvent:

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2011-05-27 21:12:25 +0000
+++ lib/lp/registry/model/distribution.py	2011-06-07 05:39:33 +0000
@@ -646,10 +646,14 @@
         """See `IBugTarget`."""
         return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
 
-    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
-        """See `IBugTarget`."""
+    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None):
+        """See IBugTarget."""
+        # Circular fail.
+        from lp.bugs.model.bugsummary import BugSummary
         return get_bug_tags_open_count(
-            BugTask.distribution == self, user, wanted_tags=wanted_tags)
+            And(BugSummary.distribution_id == self.id,
+                BugSummary.sourcepackagename_id == None),
+            user, tag_limit=tag_limit, include_tags=include_tags)
 
     def getMirrorByName(self, name):
         """See `IDistribution`."""

=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
--- lib/lp/registry/model/distributionsourcepackage.py	2011-05-12 04:18:32 +0000
+++ lib/lp/registry/model/distributionsourcepackage.py	2011-06-07 05:39:33 +0000
@@ -486,12 +486,14 @@
         """See `IBugTarget`."""
         return self.distribution.getUsedBugTags()
 
-    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
-        """See `IBugTarget`."""
+    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None):
+        """See IBugTarget."""
+        # Circular fail.
+        from lp.bugs.model.bugsummary import BugSummary
         return get_bug_tags_open_count(
-            And(BugTask.distribution == self.distribution,
-                BugTask.sourcepackagename == self.sourcepackagename),
-            user, wanted_tags=wanted_tags)
+            And(BugSummary.distribution == self.distribution,
+                BugSummary.sourcepackagename == self.sourcepackagename),
+            user, tag_limit=tag_limit, include_tags=include_tags)
 
     def _getOfficialTagClause(self):
         return self.distribution._getOfficialTagClause()

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-05-31 15:45:19 +0000
+++ lib/lp/registry/model/distroseries.py	2011-06-07 05:39:33 +0000
@@ -28,6 +28,7 @@
     StringCol,
     )
 from storm.locals import (
+    And,
     Desc,
     Join,
     SQL,
@@ -853,10 +854,16 @@
         """See `IHasBugs`."""
         return get_bug_tags("BugTask.distroseries = %s" % sqlvalues(self))
 
-    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
-        """See `IHasBugs`."""
+    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None):
+        """See IBugTarget."""
+        # Circular fail.
+        from lp.bugs.model.bugsummary import BugSummary
         return get_bug_tags_open_count(
-            BugTask.distroseries == self, user, wanted_tags=wanted_tags)
+            And(
+                BugSummary.distroseries_id == self.id,
+                BugSummary.sourcepackagename_id == None
+                ),
+            user, tag_limit=tag_limit, include_tags=include_tags)
 
     @property
     def has_any_specifications(self):

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2011-05-27 21:12:25 +0000
+++ lib/lp/registry/model/product.py	2011-06-07 05:39:33 +0000
@@ -799,10 +799,13 @@
         """See `IBugTarget`."""
         return get_bug_tags("BugTask.product = %s" % sqlvalues(self))
 
-    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
-        """See `IBugTarget`."""
+    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None):
+        """See IBugTarget."""
+        # Circular fail.
+        from lp.bugs.model.bugsummary import BugSummary
         return get_bug_tags_open_count(
-            BugTask.product == self, user, wanted_tags=wanted_tags)
+            BugSummary.product_id == self.id,
+            user, tag_limit=tag_limit, include_tags=include_tags)
 
     series = SQLMultipleJoin('ProductSeries', joinColumn='product',
         orderBy='name')

=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py	2011-05-27 21:12:25 +0000
+++ lib/lp/registry/model/productseries.py	2011-06-07 05:39:33 +0000
@@ -446,10 +446,13 @@
         """See IBugTarget."""
         return get_bug_tags("BugTask.productseries = %s" % sqlvalues(self))
 
-    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
+    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None):
         """See IBugTarget."""
+        # Circular fail.
+        from lp.bugs.model.bugsummary import BugSummary
         return get_bug_tags_open_count(
-            BugTask.productseries == self, user, wanted_tags=wanted_tags)
+            BugSummary.productseries_id == self.id, user, tag_limit=tag_limit,
+            include_tags=include_tags)
 
     def createBug(self, bug_params):
         """See IBugTarget."""

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2011-04-26 16:22:11 +0000
+++ lib/lp/registry/model/projectgroup.py	2011-06-07 05:39:33 +0000
@@ -343,14 +343,16 @@
         return get_bug_tags(
             "BugTask.product IN (%s)" % ",".join(product_ids))
 
-    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
-        """See `IHasBugs`."""
-        if not self.products:
-            return []
+    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None):
+        """See IBugTarget."""
+        # Circular fail.
+        from lp.bugs.model.bugsummary import BugSummary
         product_ids = [product.id for product in self.products]
+        if not product_ids:
+            return {}
         return get_bug_tags_open_count(
-            BugTask.productID.is_in(product_ids), user,
-            wanted_tags=wanted_tags)
+            BugSummary.product_id.is_in(product_ids),
+            user, tag_limit=tag_limit, include_tags=include_tags)
 
     def _getBugTaskContextClause(self):
         """See `HasBugsBase`."""

=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py	2011-05-14 15:03:04 +0000
+++ lib/lp/registry/model/sourcepackage.py	2011-06-07 05:39:33 +0000
@@ -494,12 +494,14 @@
         """See `IBugTarget`."""
         return self.distroseries.getUsedBugTags()
 
-    def getUsedBugTagsWithOpenCounts(self, user, wanted_tags=None):
-        """See `IBugTarget`."""
+    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None):
+        """See IBugTarget."""
+        # Circular fail.
+        from lp.bugs.model.bugsummary import BugSummary
         return get_bug_tags_open_count(
-            And(BugTask.distroseries == self.distroseries,
-                BugTask.sourcepackagename == self.sourcepackagename),
-            user, wanted_tags=wanted_tags)
+            And(BugSummary.distroseries == self.distroseries,
+                BugSummary.sourcepackagename == self.sourcepackagename),
+            user, tag_limit=tag_limit, include_tags=include_tags)
 
     @property
     def max_bug_heat(self):