Merge lp:~edwin-grubbs/launchpad/bug-436974-series-timeouts into lp:launchpad

[Edwin Grubbs]

[Edwin Grubbs (edwin-grubbs)]

Summary
-------

Optimize the $productseries/+series page.

Implementation details
----------------------

Eliminated a query for product release files by making it lazy load,
since that data is not used on the +series page.

Improved the query for BugTask.getStatusCountsForProductSeries(), which
is called for every active series in the product.

Tests
-----

./bin/test -vv -t xx-productseries-series.txt

Demo and Q/A
------------

* Open http://launchpad.dev/bzr/+series
  * This should not timeout on staging or edge.

[Michael Nelson (michael.nelson)]

Hi Edwin,

I've marked this as Needs Fixing because there are a few things that look like oversights - but I might be wrong. Other than that, just a few comments. See below.

Thanks!

> === modified file 'lib/lp/bugs/model/bugtask.py'
> --- lib/lp/bugs/model/bugtask.py 2009-10-01 19:50:54 +0000
> +++ lib/lp/bugs/model/bugtask.py 2009-10-15 09:32:02 +0000
> @@ -2021,22 +2021,37 @@
> """See `IBugTaskSet`."""
> bug_privacy_filter = get_bug_privacy_filter(user)
> if bug_privacy_filter != "":
> - bug_privacy_filter = ' AND ' + bug_privacy_filter
> + bug_privacy_filter = 'AND ' + bug_privacy_filter
> cur = cursor()
> - condition = """
> - (BugTask.productseries = %s
> - OR Milestone.productseries = %s)
> - """ % sqlvalues(product_series, product_series)
> +
> + # The union is actually much faster than a LEFT JOIN with the
> + # Milestone table, since postgres optimizes it to perform index
> + # scans instead of sequential scans on the BugTask table.

Wow - was there any way you were able to verify that locally? That's
really interesting.

> query = """
> - SELECT BugTask.status, count(*)
> - FROM BugTask
> - JOIN Bug ON BugTask.bug = Bug.id
> - LEFT JOIN Milestone ON BugTask.milestone = Milestone.id
> - WHERE
> - %s
> - %s
> - GROUP BY BugTask.status
> - """ % (condition, bug_privacy_filter)
> + SELECT status, count(*)
> + FROM (
> + SELECT BugTask.status
> + FROM BugTask
> + JOIN Bug ON BugTask.bug = Bug.id
> + WHERE
> + BugTask.productseries = %(series)s
> + %(privacy)s
> +
> + UNION ALL
> +
> + SELECT BugTask.status
> + FROM BugTask
> + JOIN Bug ON BugTask.bug = Bug.id
> + JOIN Milestone ON BugTask.milestone = Milestone.id
> + WHERE
> + BugTask.productseries IS NULL
> + AND Milestone.productseries = %(series)s
> + %(privacy)s
> + ) AS subquery
> + GROUP BY status
> + """ % dict(series=quote(product_series),
> + privacy=bug_privacy_filter)
> +
> cur.execute(query)
> return cur.fetchall()
>
>
> === modified file 'lib/lp/registry/browser/product.py'
> --- lib/lp/registry/browser/product.py 2009-10-08 15:54:09 +0000
> +++ lib/lp/registry/browser/product.py 2009-10-15 09:32:02 +0000
> @@ -558,14 +558,28 @@
> def __init__(self, product):
> self.product = product
> self.serieses = []
> - self.series_by_id = {}
> + for series in self.product.serieses:
> + series_with_releases = SeriesWithReleases(series, parent=self)
> + self.serieses.append(series_with_releases)
> + if self.product.development_focus == series:
> + self.development_focus = series_with_releases
>
> - def...

review: Needs Fixing (code)

[Edwin Grubbs (edwin-grubbs)]

Hi Michael,

Thanks for the review. Incremental diff included.

On Thu, Oct 15, 2009 at 5:51 AM, Michael Nelson
<email address hidden> wrote:
> Review: Needs Fixing code
> Hi Edwin,
>
> I've marked this as Needs Fixing because there are a few things that look like oversights - but I might be wrong. Other than that, just a few comments. See below.
>
> Thanks!
>
>> === modified file 'lib/lp/bugs/model/bugtask.py'
>> --- lib/lp/bugs/model/bugtask.py      2009-10-01 19:50:54 +0000
>> +++ lib/lp/bugs/model/bugtask.py      2009-10-15 09:32:02 +0000
>> @@ -2021,22 +2021,37 @@
>>          """See `IBugTaskSet`."""
>>          bug_privacy_filter = get_bug_privacy_filter(user)
>>          if bug_privacy_filter != "":
>> -            bug_privacy_filter = ' AND ' + bug_privacy_filter
>> +            bug_privacy_filter = 'AND ' + bug_privacy_filter
>>          cur = cursor()
>> -        condition = """
>> -            (BugTask.productseries = %s
>> -                 OR Milestone.productseries = %s)
>> -            """ % sqlvalues(product_series, product_series)
>> +
>> +        # The union is actually much faster than a LEFT JOIN with the
>> +        # Milestone table, since postgres optimizes it to perform index
>> +        # scans instead of sequential scans on the BugTask table.
>
> Wow - was there any way you were able to verify that locally? That's
> really interesting.

It actually runs a little bit slower locally, but on staging's db it runs 50
to 170 times as fast. I can show you the output of EXPLAIN ANALYZE if
you are interested.

>>          query = """
>> -            SELECT BugTask.status, count(*)
>> -            FROM BugTask
>> -                JOIN Bug ON BugTask.bug = Bug.id
>> -                LEFT JOIN Milestone ON BugTask.milestone = Milestone.id
>> -            WHERE
>> -                %s
>> -                %s
>> -            GROUP BY BugTask.status
>> -            """ % (condition, bug_privacy_filter)
>> +            SELECT status, count(*)
>> +            FROM (
>> +                SELECT BugTask.status
>> +                FROM BugTask
>> +                    JOIN Bug ON BugTask.bug = Bug.id
>> +                WHERE
>> +                    BugTask.productseries = %(series)s
>> +                    %(privacy)s
>> +
>> +                UNION ALL
>> +
>> +                SELECT BugTask.status
>> +                FROM BugTask
>> +                    JOIN Bug ON BugTask.bug = Bug.id
>> +                    JOIN Milestone ON BugTask.milestone = Milestone.id
>> +                WHERE
>> +                    BugTask.productseries IS NULL
>> +                    AND Milestone.productseries = %(series)s
>> +                    %(privacy)s
>> +                ) AS subquery
>> +            GROUP BY status
>> +            """ % dict(series=quote(product_series),
>> +                       privacy=bug_privacy_filter)
>> +
>>          cur.execute(query)
>>          return cur.fetchall()
>>
>>
>> === modified file 'lib/lp/registry/browser/product.py'
>> --- lib/lp/registry/browser/product.py        2009-10-08 15:54:09 +0000
>> +++ lib/lp/registry/browser/product.py        2009-10-15 09:32:02 +0000
>> @@ -558,14 +558,28 @@
>>      def __...

[Michael Nelson (michael.nelson)]

> Hi Michael,
>
> Thanks for the review. Incremental diff included.
>

Thanks for making me laugh with your comments :)

Just a typo in your comment below. Great work!

> === modified file 'lib/lp/registry/browser/product.py'
> --- lib/lp/registry/browser/product.py 2009-10-13 02:44:40 +0000
> +++ lib/lp/registry/browser/product.py 2009-10-15 15:38:50 +0000
> @@ -643,22 +640,25 @@
> self.parent = parent
> self._files = None
>
> - @cachedproperty
> + @property
> def files(self):
> + """Cache the release files for all the releases in the product."""
> if self._files is None:
> # Get all of the files for all of the releases. The query
> # returns all releases sorted properly.
> product = self.parent.parent
> - decorated_releases = product.release_by_id.values()
> + release_delegates = product.release_by_id.values()
> files = getUtility(IProductReleaseSet).getFilesForReleases(
> - decorated_releases)
> - for decorated_release in decorated_releases:
> - decorated_release._files = []
> + release_delegates)
> + for release_delegate in release_delegates:
> + release_delegate._files = []
> for file in files:
> id = file.productrelease.id
> - decorated_release = product.release_by_id[id]
> - decorated_release._files.append(file)
> + release_delegate = product.release_by_id[id]
> + release_delegate._files.append(file)
>
> + # self._files was set above, since self is actually in the
> + # decorated_releases variable.

Should be release_delegates now ^^^^ to avoid confusion :)

review: Approve (code)

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2009-10-01 19:50:54 +0000
+++ lib/lp/bugs/model/bugtask.py	2009-10-16 14:08:13 +0000
@@ -2021,22 +2021,37 @@
         """See `IBugTaskSet`."""
         bug_privacy_filter = get_bug_privacy_filter(user)
         if bug_privacy_filter != "":
-            bug_privacy_filter = ' AND ' + bug_privacy_filter
+            bug_privacy_filter = 'AND ' + bug_privacy_filter
         cur = cursor()
-        condition = """
-            (BugTask.productseries = %s
-                 OR Milestone.productseries = %s)
-            """ % sqlvalues(product_series, product_series)
+
+        # The union is actually much faster than a LEFT JOIN with the
+        # Milestone table, since postgres optimizes it to perform index
+        # scans instead of sequential scans on the BugTask table.
         query = """
-            SELECT BugTask.status, count(*)
-            FROM BugTask
-                JOIN Bug ON BugTask.bug = Bug.id
-                LEFT JOIN Milestone ON BugTask.milestone = Milestone.id
-            WHERE
-                %s
-                %s
-            GROUP BY BugTask.status
-            """ % (condition, bug_privacy_filter)
+            SELECT status, count(*)
+            FROM (
+                SELECT BugTask.status
+                FROM BugTask
+                    JOIN Bug ON BugTask.bug = Bug.id
+                WHERE
+                    BugTask.productseries = %(series)s
+                    %(privacy)s
+
+                UNION ALL
+
+                SELECT BugTask.status
+                FROM BugTask
+                    JOIN Bug ON BugTask.bug = Bug.id
+                    JOIN Milestone ON BugTask.milestone = Milestone.id
+                WHERE
+                    BugTask.productseries IS NULL
+                    AND Milestone.productseries = %(series)s
+                    %(privacy)s
+                ) AS subquery
+            GROUP BY status
+            """ % dict(series=quote(product_series),
+                       privacy=bug_privacy_filter)
+
         cur.execute(query)
         return cur.fetchall()
 
 
=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2009-10-08 15:54:09 +0000
+++ lib/lp/registry/browser/product.py	2009-10-16 14:08:13 +0000
@@ -549,8 +549,9 @@
     cached locally and simply returned.
     """
 
-    # These need to be predeclared to avoid delegates taking them
-    # over.
+    # `serieses` and `development_focus` need to be declared as class
+    # attributes so that this class will not delegate the actual instance
+    # variables to self.product, which would bypass the caching.
     serieses = None
     development_focus = None
     delegates(IProduct, 'product')
@@ -558,17 +559,24 @@
     def __init__(self, product):
         self.product = product
         self.serieses = []
-        self.series_by_id = {}
-
-    def setSeries(self, serieses):
-        """Set the serieses to the provided collection."""
-        self.serieses = serieses
-        self.series_by_id = dict(
-            (series.id, series) for series in self.serieses)
-
-    def getSeriesById(self, id):
-        """Look up and return a ProductSeries by id."""
-        return self.series_by_id[id]
+        for series in self.product.serieses:
+            series_with_releases = SeriesWithReleases(series, parent=self)
+            self.serieses.append(series_with_releases)
+            if self.product.development_focus == series:
+                self.development_focus = series_with_releases
+
+        # Get all of the releases for all of the serieses in a single
+        # query.  The query sorts the releases properly so we know the
+        # resulting list is sorted correctly.
+        series_by_id = dict((series.id, series) for series in self.serieses)
+        self.release_by_id = {}
+        milestones_and_releases = list(
+            self.product.getMilestonesAndReleases())
+        for milestone, release in milestones_and_releases:
+            series = series_by_id[milestone.productseries.id]
+            release_delegate = ReleaseWithFiles(release, parent=series)
+            series.addRelease(release_delegate)
+            self.release_by_id[release.id] = release_delegate
 
 
 class SeriesWithReleases:
@@ -579,13 +587,17 @@
     cached locally and simply returned.
     """
 
-    # These need to be predeclared to avoid delegates taking them
-    # over.
+    # `parent` and `releases` need to be declared as class attributes so that
+    # this class will not delegate the actual instance variables to
+    # self.series, which would bypass the caching for self.releases and would
+    # raise an AttributeError for self.parent.
+    parent = None
     releases = None
     delegates(IProductSeries, 'series')
 
-    def __init__(self, series):
+    def __init__(self, series, parent):
         self.series = series
+        self.parent = parent
         self.releases = []
 
     def addRelease(self, release):
@@ -617,17 +629,37 @@
     cached locally and simply returned.
     """
 
-    # These need to be predeclared to avoid delegates taking them
-    # over.
-    files = None
+    # `parent` needs to be declared as class attributes so that
+    # this class will not delegate the actual instance variables to
+    # self.release, which would raise an AttributeError.
+    parent = None
     delegates(IProductRelease, 'release')
 
-    def __init__(self, release):
+    def __init__(self, release, parent):
         self.release = release
-        self.files = []
-
-    def addFile(self, file):
-        self.files.append(file)
+        self.parent = parent
+        self._files = None
+
+    @property
+    def files(self):
+        """Cache the release files for all the releases in the product."""
+        if self._files is None:
+            # Get all of the files for all of the releases.  The query
+            # returns all releases sorted properly.
+            product = self.parent.parent
+            release_delegates = product.release_by_id.values()
+            files = getUtility(IProductReleaseSet).getFilesForReleases(
+                release_delegates)
+            for release_delegate in release_delegates:
+                release_delegate._files = []
+            for file in files:
+                id = file.productrelease.id
+                release_delegate = product.release_by_id[id]
+                release_delegate._files.append(file)
+
+        # self._files was set above, since self is actually in the
+        # release_delegates variable.
+        return self._files
 
     @property
     def name_with_codename(self):
@@ -653,41 +685,7 @@
         Decorated classes are created, and they contain cached data
         obtained with a few queries rather than many iterated queries.
         """
-        # Create the decorated product and set the list of series.
-        original_product = self.context
-
-        product_with_series = ProductWithSeries(original_product)
-        serieses = []
-        for series in original_product.serieses:
-            series_with_releases = SeriesWithReleases(series)
-            serieses.append(series_with_releases)
-            if original_product.development_focus == series:
-                product_with_series.development_focus = series_with_releases
-
-        product_with_series.setSeries(serieses)
-
-        # Get all of the releases for all of the serieses in a single
-        # query.  The query sorts the releases properly so we know the
-        # resulting list is sorted correctly.
-        release_by_id = {}
-        milestones_and_releases = list(
-            original_product.getMilestonesAndReleases())
-        for milestone, release in milestones_and_releases:
-            series = product_with_series.getSeriesById(
-                milestone.productseries.id)
-            decorated_release = ReleaseWithFiles(release)
-            series.addRelease(decorated_release)
-            release_by_id[release.id] = decorated_release
-
-        # Get all of the files for all of the releases.  The query
-        # returns all releases sorted properly.
-        releases = [release for milestone, release in milestones_and_releases]
-        files = getUtility(IProductReleaseSet).getFilesForReleases(releases)
-        for file in files:
-            release = release_by_id[file.productrelease.id]
-            release.addFile(file)
-
-        return product_with_series
+        return ProductWithSeries(self.context)
 
     def deleteFiles(self, releases):
         """Delete the selected files from the set of releases.