Merge into devel : bug-436974-series-timeouts : Code : Launchpad itself

Status:

Merged

Approved by:

Edwin Grubbs on 2009-10-16

Approved revision:

no longer in the source branch.

Merged at revision:

not available

Proposed branch:

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

Merge into:

lp:launchpad

Diff against target:

217 lines

2 files modified

lib/lp/bugs/model/bugtask.py (+29/-14)
lib/lp/registry/browser/product.py (+57/-59)

To merge this branch:

bzr merge lp:~edwin-grubbs/launchpad/bug-436974-series-timeouts

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Nelson (community)	code	2009-10-15	Approve on 2009-10-16
Review via email: mp+13397@code.launchpad.net

Revision history for this message

Edwin Grubbs (edwin-grubbs) wrote on 2009-10-15:

#

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.

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2009-10-15:

#

Download full text (10.5 KiB)

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...

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 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)
>  
> +        # 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.
> +        self.release_by_id = {}
> +        milestones_and_releases = list(
> +            self.product.getMilestonesAndReleases())
> +        for milestone, release in milestones_and_releases:
> +            series = self.getSeriesById(
> +                milestone.productseries.id)
> +            decorated_release = ReleaseWithFiles(release, parent=series)
> +            series.addRelease(decorated_release)
> +            self.release_by_id[release.id] = decorated_release
> +

Is it worth making series, product_development_focus, series_by_id and
release_by_id properties/cachedproperties instead? Up to you - as it
would mean adding documentation for them - which would be a good thing,
but extra work.

>      def getSeriesById(self, id):
>          """Look up and return a ProductSeries by id."""
>          return self.series_by_id[id]
> @@ -581,11 +595,13 @@
>  
>      # These need to be predeclared to avoid delegates taking them
>      # over.
> +    parent = None

OK, I don't quite understand what you mean by that comment. So if
IProductSeries declares a parent attribute, then doing
series_with_releases.parent would automatically be delegated to
series_with_releases.series.parent - if you didn't first declare parent
as an attribute here? Is that right? If so (or otherwise!), it might be
worth expanding the comment so that knowledge of lazr.delegates isn't
required to understand it?

>      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):
> @@ -619,15 +635,31 @@
>  
>      # These need to be predeclared to avoid delegates taking them
>      # over.
> -    files = None
> +    parent = None

Hrm, but you wouldn't want the expanded comment twice. Although if it's
just one extra line it's not too bad.

>      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
> +
> +    @cachedproperty
> +    def files(self):
> +        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()

Does someone reading this method need to know that the releases are
decorated? Is it because you're decorating it with the _files attribute here? if not, perhaps consider just releases.

> +            files = getUtility(IProductReleaseSet).getFilesForReleases(
> +                decorated_releases)
> +            for decorated_release in decorated_releases:
> +                decorated_release._files = []
> +            for file in files:
> +                id = file.productrelease.id
> +                decorated_release = product.release_by_id[id]
> +                decorated_release._files.append(file)
> +
> +        return self._files

Hmm... I'm confused on a number of points here.

First, self._files is never populated (it's decorated_release._files that
is being populated) and yet it's still returned? Did you mean to do
self._files = getUtility(....).getFilesForReleases() above? or did I miss
something? If this is the case, a test for ReleaseWithFiles.files would have
picked this up too.

Second, are you meaning to double cache the files property?
I'd assume you could either use cachedproperty OR cache it yourself
using _files, but why both?

>  
>      @property
>      def name_with_codename(self):
> @@ -653,40 +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)
> -
> +        product_with_series = ProductWithSeries(self.context)
>          return product_with_series

Nice. You could even just do:
            return ProductWithSeries(self.context)

>  
>      def deleteFiles(self, releases):
> @@ -737,11 +736,15 @@
>      @cachedproperty
>      def latest_release_with_download_files(self):
>          """Return the latest release with download files."""
> -        for series in self.sorted_series_list:
> -            for release in series.releases:
> -                if len(list(release.files)) > 0:
> -                    return release
> -        return None
> +        try:
> +            for series in self.sorted_series_list:
> +                for release in series.releases:
> +                    if len(list(release.files)) > 0:
> +                        return release
> +            return None
> +        except AttributeError, e:
> +            import sys
> +            raise ValueError, e, sys.exc_info()[2]

Hrm - seeing a change like this without a corresponding test change or
comment scares me :). I'm guessing you didn't mean to leave this here
and it's left over from something you were testing while working on this
branch? If so, great - just get rid of it. If not - then please document
latest_release_with_download_files.

Cheers,
Michael

>  
>  
>  class ProductView(HasAnnouncementsView, SortSeriesMixin, FeedsMixin,

-- 
Michael

review: Needs Fixing (code)

Revision history for this message

Edwin Grubbs (edwin-grubbs) wrote on 2009-10-15:

#

Download full text (17.4 KiB)

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 __...

Hi Michael,

Thanks for the review. Incremental diff included.

On Thu, Oct 15, 2009 at 5:51 AM, Michael Nelson
<michael.nelson@canonical.com> 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 __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 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)
>>
>> +        # 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.
>> +        self.release_by_id = {}
>> +        milestones_and_releases = list(
>> +            self.product.getMilestonesAndReleases())
>> +        for milestone, release in milestones_and_releases:
>> +            series = self.getSeriesById(
>> +                milestone.productseries.id)
>> +            decorated_release = ReleaseWithFiles(release, parent=series)
>> +            series.addRelease(decorated_release)
>> +            self.release_by_id[release.id] = decorated_release
>> +
>
> Is it worth making series, product_development_focus, series_by_id and
> release_by_id properties/cachedproperties instead? Up to you - as it
> would mean adding documentation for them - which would be a good thing,
> but extra work.

I looked into this. The problem is that ReleaseWithFiles is the only thing
using the release_by_id property, and ReleaseWithFiles is actually created
in that for-loop. It is much more efficient to create the release_by_id
attribute in that for-loop rather than crawling the product.series.releases
tree structure later on.

I was able to turn series_by_id into a local variable since getSeriesById()
isn't actually used anywhere.

>>      def getSeriesById(self, id):
>>          """Look up and return a ProductSeries by id."""
>>          return self.series_by_id[id]
>> @@ -581,11 +595,13 @@
>>
>>      # These need to be predeclared to avoid delegates taking them
>>      # over.
>> +    parent = None
>
> OK, I don't quite understand what you mean by that comment. So if
> IProductSeries declares a parent attribute, then doing
> series_with_releases.parent would automatically be delegated to
> series_with_releases.series.parent - if you didn't first declare parent
> as an attribute here? Is that right? If so (or otherwise!), it might be
> worth expanding the comment so that knowledge of lazr.delegates isn't
> required to understand it?

I expanded the comment, so it should be much clearer now.

>>      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):
>> @@ -619,15 +635,31 @@
>>
>>      # These need to be predeclared to avoid delegates taking them
>>      # over.
>> -    files = None
>> +    parent = None
>
> Hrm, but you wouldn't want the expanded comment twice. Although if it's
> just one extra line it's not too bad.

I did that here, too.

>>      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
>> +
>> +    @cachedproperty
>> +    def files(self):
>> +        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()
>
> Does someone reading this method need to know that the releases are
> decorated? Is it because you're decorating it with the _files attribute here? if not, perhaps consider just releases.

Yes, I find it confusing when I don't know which kind of object it is,
and setting _files only makes sense
for the delegate. I realize now that someone might confuse
decorated_releases with python decorators, so I am
changing decorated_releases to release_delegates.

>> +            files = getUtility(IProductReleaseSet).getFilesForReleases(
>> +                decorated_releases)
>> +            for decorated_release in decorated_releases:
>> +                decorated_release._files = []
>> +            for file in files:
>> +                id = file.productrelease.id
>> +                decorated_release = product.release_by_id[id]
>> +                decorated_release._files.append(file)
>> +
>> +        return self._files
>
> Hmm... I'm confused on a number of points here.
>
> First, self._files is never populated (it's decorated_release._files that
> is being populated) and yet it's still returned? Did you mean to do
> self._files = getUtility(....).getFilesForReleases() above? or did I miss
> something? If this is the case, a test for ReleaseWithFiles.files would have
> picked this up too.

self is also in the decorated_releases variable, so its _files attribute gets
set in the for-loop above as well.

> Second, are you meaning to double cache the files property?
> I'd assume you could either use cachedproperty OR cache it yourself
> using _files, but why both?

Er, I'm paid on a per-cache basis. Fixed.

>>
>>      @property
>>      def name_with_codename(self):
>> @@ -653,40 +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)
>> -
>> +        product_with_series = ProductWithSeries(self.context)
>>          return product_with_series
>
> Nice. You could even just do:
>            return ProductWithSeries(self.context)

I'm also paid per line of code.

>>
>>      def deleteFiles(self, releases):
>> @@ -737,11 +736,15 @@
>>      @cachedproperty
>>      def latest_release_with_download_files(self):
>>          """Return the latest release with download files."""
>> -        for series in self.sorted_series_list:
>> -            for release in series.releases:
>> -                if len(list(release.files)) > 0:
>> -                    return release
>> -        return None
>> +        try:
>> +            for series in self.sorted_series_list:
>> +                for release in series.releases:
>> +                    if len(list(release.files)) > 0:
>> +                        return release
>> +            return None
>> +        except AttributeError, e:
>> +            import sys
>> +            raise ValueError, e, sys.exc_info()[2]
>
> Hrm - seeing a change like this without a corresponding test change or
> comment scares me :). I'm guessing you didn't mean to leave this here
> and it's left over from something you were testing while working on this
> branch? If so, great - just get rid of it. If not - then please document
> latest_release_with_download_files.

Doh, this was a debugging workaround to get the traceback for the AttributeError
that the zope publisher has a nasty habitat of throwing away and replacing with
a not found error.

Incremental diff:
{{{
=== 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
@@ -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')
@@ -564,26 +565,19 @@
             if self.product.development_focus == series:
                 self.development_focus = series_with_releases

-        self.series_by_id = dict(
-            (series.id, series) for series in self.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.
+        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 = self.getSeriesById(
-                milestone.productseries.id)
+            series = series_by_id[milestone.productseries.id]
             decorated_release = ReleaseWithFiles(release, parent=series)
             series.addRelease(decorated_release)
             self.release_by_id[release.id] = decorated_release

-    def getSeriesById(self, id):
-        """Look up and return a ProductSeries by id."""
-        return self.series_by_id[id]
-

class SeriesWithReleases:
     """A decorated series that includes releases.
@@ -593,8 +587,10 @@
     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')
@@ -633,8 +629,9 @@
     cached locally and simply returned.
     """

-    # These need to be predeclared to avoid delegates taking them
-    # over.
+    # `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')

@@ -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.
         return self._files

@property
@@ -685,8 +685,7 @@
         Decorated classes are created, and they contain cached data
         obtained with a few queries rather than many iterated queries.
         """
-        product_with_series = ProductWithSeries(self.context)
-        return product_with_series
+        return ProductWithSeries(self.context)

def deleteFiles(self, releases):
         """Delete the selected files from the set of releases.
@@ -736,15 +735,11 @@
     @cachedproperty
     def latest_release_with_download_files(self):
         """Return the latest release with download files."""
-        try:
-            for series in self.sorted_series_list:
-                for release in series.releases:
-                    if len(list(release.files)) > 0:
-                        return release
-            return None
-        except AttributeError, e:
-            import sys
-            raise ValueError, e, sys.exc_info()[2]
+        for series in self.sorted_series_list:
+            for release in series.releases:
+                if len(list(release.files)) > 0:
+                    return release
+        return None

class ProductView(HasAnnouncementsView, SortSeriesMixin, FeedsMixin,

}}}

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2009-10-16:

#

> 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)

Launchpad itself

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

Commit message

Description of the change

Preview Diff

Subscribers

 === 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.