launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/review-licence-timeout-1034908 into lp:launchpad.

Commit message:
Reduce the query count on the +review-licences page by bulk loading and pre-caching the required information.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1034908 in Launchpad itself: "timeout on review-licenses"
  https://bugs.launchpad.net/launchpad/+bug/1034908

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/review-licence-timeout-1034908/+merge/128153

== Implementation ==

Refactor some previous product bulk loading and pre caching code used for searches and also use it when loading products for review.

The Review Licences view did use a base class which provided some caching capability for product series and product release info, but this was just for a single product as would be displayed on the product index page for example. Here, a TAL macro was repeatedly evaluated for each product, so such caching was no good. The caching had to be done in bulk in the method which loaded the products for review, prior to them being sent to the view for rendering.

The implementation relies on being able to poke stuff into cached properties. Some of the model attributes (Product.series and ProductRelease.files) were defined as type SQLMultipleJoin. These were renamed with an underscore and a new cachedproperty introduced.

A new property 'has_download_files' was added to the view. Previously, the implementation loaded an ordered series list and then got the actual latest file. We don't care about all that - we just want to know if there is a file.

Before these changes, the view executed several queries per product to load:
- commercial subscriptions
- series info
- release info
- file info

A test rendering the view for 100 products executed minimum 200 odd queries. Now less than 25 are used.

== Tests ==

The changes are internal - the content of the +review-licences page is unchanged, as are the external APIs used elsewhere. Add a test to check the query count when rendering the +review-licences view.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/product.py
  lib/lp/registry/browser/tests/test_product.py
  lib/lp/registry/model/product.py
  lib/lp/registry/model/productrelease.py
  lib/lp/registry/model/productseries.py
  lib/lp/registry/templates/product-listing-for-review.pt
-- 
https://code.launchpad.net/~wallyworld/launchpad/review-licence-timeout-1034908/+merge/128153
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/review-licence-timeout-1034908 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2012-10-04 15:54:06 +0000
+++ lib/lp/registry/browser/product.py	2012-10-05 03:29:36 +0000
@@ -917,6 +917,16 @@
                     return release
         return None
 
+    @cachedproperty
+    def has_download_files(self):
+        for series in self.context.series:
+            if series.status == SeriesStatus.OBSOLETE:
+                continue
+            for release in series.releases:
+                if len(list(release.files)) > 0:
+                    return True
+        return False
+
 
 class ProductView(PillarViewMixin, HasAnnouncementsView, SortSeriesMixin,
                   FeedsMixin, ProductDownloadFileMixin):

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2012-10-04 17:37:33 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2012-10-05 03:29:36 +0000
@@ -10,7 +10,10 @@
     HTMLContains,
     Tag,
     )
-from testtools.matchers import Not
+from testtools.matchers import (
+    LessThan,
+    Not,
+    )
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -34,7 +37,9 @@
     IProductSet,
     License,
     )
+from lp.registry.model.product import Product
 from lp.services.config import config
+from lp.services.database.lpstorm import IStore
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
@@ -42,10 +47,15 @@
     login_celebrity,
     login_person,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.fixture import DemoMode
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import find_tag_by_id
 from lp.testing.service_usage_helpers import set_service_usage
 from lp.testing.views import (
@@ -414,7 +424,7 @@
 class ProductSetReviewLicensesViewTestCase(TestCaseWithFactory):
     """Tests the ProductSetReviewLicensesView."""
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         super(ProductSetReviewLicensesViewTestCase, self).setUp()
@@ -503,6 +513,19 @@
             'Y.lp.app.choice.addBinaryChoice' in str(
                 content.find(id='fnord-edit-license-approved').parent))
 
+    def test_review_licence_query_count(self):
+        # Ensure the query count is not O(n).
+        for _ in range(100):
+            product = self.factory.makeProduct()
+            for _ in range(5):
+                self.factory.makeProductReleaseFile(product=product)
+        IStore(Product).reset()
+        with StormStatementRecorder() as recorder:
+            view = create_initialized_view(
+                self.product_set, '+review-licenses', principal=self.user)
+            view.render()
+            self.assertThat(recorder, HasQueryCount(LessThan(25)))
+
 
 class TestProductRdfView(BrowserTestCase):
     """Test the Product RDF view."""

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-10-04 17:37:33 +0000
+++ lib/lp/registry/model/product.py	2012-10-05 03:29:36 +0000
@@ -157,6 +157,7 @@
     License,
     LicenseStatus,
     )
+from lp.registry.interfaces.productrelease import IProductReleaseSet
 from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.model.announcement import MakesAnnouncements
 from lp.registry.model.commercialsubscription import CommercialSubscription
@@ -1021,9 +1022,13 @@
         """Customize `search_params` for this product.."""
         search_params.setProduct(self)
 
-    series = SQLMultipleJoin('ProductSeries', joinColumn='product',
+    _series = SQLMultipleJoin('ProductSeries', joinColumn='product',
         orderBy='name')
 
+    @cachedproperty
+    def series(self):
+        return self._series
+
     @property
     def active_or_packaged_series(self):
         store = Store.of(self)
@@ -1109,26 +1114,12 @@
         from lp.registry.model.distributionsourcepackage import (
             DistributionSourcePackage,
             )
-        store = IStore(Packaging)
-        origin = [
-            Packaging,
-            Join(SourcePackageName,
-                 Packaging.sourcepackagename == SourcePackageName.id),
-            Join(ProductSeries, Packaging.productseries == ProductSeries.id),
-            Join(DistroSeries, Packaging.distroseries == DistroSeries.id),
-            Join(Distribution, DistroSeries.distribution == Distribution.id),
-            ]
-        result = store.using(*origin).find(
-            (SourcePackageName, Distribution),
-            ProductSeries.product == self)
-        result = result.order_by(SourcePackageName.name, Distribution.name)
-        result.config(distinct=True)
-
+        dsp_info = getDistroSourcePackages([self])
         return [
             DistributionSourcePackage(
                 sourcepackagename=sourcepackagename,
                 distribution=distro)
-            for sourcepackagename, distro in result]
+            for sourcepackagename, distro, product_id in dsp_info]
 
     @cachedproperty
     def ubuntu_packages(self):
@@ -1456,22 +1447,14 @@
             And(Milestone.product == self,
                 Milestone.name == version)).one()
 
-    # XXX: jcsackett 2010-08-23 bug=620494
-    # The second clause in the order_by in this method is a bandaid
-    # on a sorting issue caused by date vs datetime conflicts in the
-    # database. A fix is coming out, but this deals with the edge
-    # case responsible for the referenced bug.
     def getMilestonesAndReleases(self):
         """See `IProduct`."""
-        store = Store.of(self)
-        result = store.find(
-            (Milestone, ProductRelease),
-            And(ProductRelease.milestone == Milestone.id,
-                Milestone.productseries == ProductSeries.id,
-                ProductSeries.product == self))
-        return result.order_by(
-            Desc(ProductRelease.datereleased),
-            Desc(Milestone.name))
+
+        def strip_product_id(row):
+            return row[0], row[1]
+
+        return DecoratedResultSet(
+            getMilestonesAndReleases([self]), strip_product_id)
 
     def packagedInDistros(self):
         return IStore(Distribution).find(
@@ -1578,6 +1561,150 @@
         return True
 
 
+def getPrecachedProducts(products, need_licences=False, need_projects=False,
+                        need_series=False, need_releases=False,
+                        role_names=None, need_role_validity=False):
+    """Load and cache product information.
+
+    :param products: the products for which to pre-cache information
+    :param need_licences: whether to cache license information
+    :param need_projects: whether to cache project information
+    :param need_series: whether to cache series information
+    :param need_releases: whether to cache release information
+    :param role_names: the role names to cache eg bug_supervisor
+    :param need_role_validity: whether to cache validity information
+    :return: a list of products
+    """
+
+    # Circular import.
+    from lp.registry.model.projectgroup import ProjectGroup
+
+    product_ids = set(obj.id for obj in products)
+    if not product_ids:
+        return
+    products_by_id = dict((product.id, product) for product in products)
+    caches = dict((product.id, get_property_cache(product))
+        for product in products)
+    for cache in caches.values():
+        if not safe_hasattr(cache, 'commercial_subscription'):
+            cache.commercial_subscription = None
+        if need_licences and  not safe_hasattr(cache, '_cached_licenses'):
+            cache._cached_licenses = []
+        if not safe_hasattr(cache, 'distrosourcepackages'):
+            cache.distrosourcepackages = []
+        if need_series and not safe_hasattr(cache, 'series'):
+            cache.series = []
+
+    from lp.registry.model.distributionsourcepackage import (
+        DistributionSourcePackage,
+        )
+
+    distrosourcepackages = getDistroSourcePackages(products)
+    for sourcepackagename, distro, product_id in distrosourcepackages:
+        cache = caches[product_id]
+        dsp = DistributionSourcePackage(
+                        sourcepackagename=sourcepackagename,
+                        distribution=distro)
+        cache.distrosourcepackages.append(dsp)
+
+    if need_series:
+        series_caches = {}
+        for series in IStore(ProductSeries).find(
+            ProductSeries,
+            ProductSeries.productID.is_in(product_ids)):
+            series_cache = get_property_cache(series)
+            if need_releases and not safe_hasattr(series_cache, 'releases'):
+                series_cache.releases = []
+
+            series_caches[series.id] = series_cache
+            cache = caches[series.productID]
+            cache.series.append(series)
+        if need_releases:
+            release_caches = {}
+            all_releases = []
+            milestones_and_releases = getMilestonesAndReleases(products)
+            for milestone, release, product_id in milestones_and_releases:
+                release_cache = get_property_cache(release)
+                release_caches[release.id] = release_cache
+                if not safe_hasattr(release_cache, 'files'):
+                    release_cache.files = []
+                all_releases.append(release)
+                series_cache = series_caches[milestone.productseries.id]
+                series_cache.releases.append(release)
+
+            prs = getUtility(IProductReleaseSet)
+            files = prs.getFilesForReleases(all_releases)
+            for file in files:
+                release_cache = release_caches[file.productrelease.id]
+                release_cache.files.append(file)
+
+    for subscription in IStore(CommercialSubscription).find(
+        CommercialSubscription,
+        CommercialSubscription.productID.is_in(product_ids)):
+        cache = caches[subscription.productID]
+        cache.commercial_subscription = subscription
+    if need_licences:
+        for license in IStore(ProductLicense).find(
+            ProductLicense,
+            ProductLicense.productID.is_in(product_ids)):
+            cache = caches[license.productID]
+            if not license.license in cache._cached_licenses:
+                cache._cached_licenses.append(license.license)
+    if need_projects:
+        bulk.load_related(ProjectGroup, products_by_id.values(), ['projectID'])
+    bulk.load_related(ProductSeries, products_by_id.values(),
+        ['development_focusID'])
+    if role_names is not None:
+        person_ids = set()
+        for attr_name in role_names:
+            person_ids.update(map(
+                lambda x: getattr(x, attr_name + 'ID'),
+                products_by_id.values()))
+        person_ids.discard(None)
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            person_ids, need_validity=need_role_validity))
+    return products
+
+
+# XXX: jcsackett 2010-08-23 bug=620494
+# The second clause in the order_by in this method is a bandaid
+# on a sorting issue caused by date vs datetime conflicts in the
+# database. A fix is coming out, but this deals with the edge
+# case responsible for the referenced bug.
+def getMilestonesAndReleases(products):
+    """Bulk load the milestone and release information for the products."""
+    store = IStore(Product)
+    product_ids = [product.id for product in products]
+    result = store.find(
+        (Milestone, ProductRelease, ProductSeries.productID),
+        And(ProductRelease.milestone == Milestone.id,
+            Milestone.productseries == ProductSeries.id,
+            ProductSeries.productID.is_in(product_ids)))
+    return result.order_by(
+        Desc(ProductRelease.datereleased),
+        Desc(Milestone.name))
+
+
+def getDistroSourcePackages(products):
+    """Bulk load the source package information for the products."""
+    store = IStore(Packaging)
+    origin = [
+        Packaging,
+        Join(SourcePackageName,
+             Packaging.sourcepackagename == SourcePackageName.id),
+        Join(ProductSeries, Packaging.productseries == ProductSeries.id),
+        Join(DistroSeries, Packaging.distroseries == DistroSeries.id),
+        Join(Distribution, DistroSeries.distribution == Distribution.id),
+        ]
+    product_ids = [product.id for product in products]
+    result = store.using(*origin).find(
+        (SourcePackageName, Distribution, ProductSeries.productID),
+        ProductSeries.productID.is_in(product_ids))
+    result = result.order_by(SourcePackageName.name, Distribution.name)
+    result.config(distinct=True)
+    return result
+
+
 class ProductSet:
     implements(IProductSet)
 
@@ -1837,49 +1964,29 @@
             Product, *conditions).config(
                 distinct=True).order_by(
                     Product.datecreated, Product.displayname)
-        return result
+
+        def eager_load(products):
+            return getPrecachedProducts(
+                products, role_names=['_owner', 'registrant'],
+                need_role_validity=True, need_licences=True,
+                need_series=True, need_releases=True)
+
+        return DecoratedResultSet(result, pre_iter_hook=eager_load)
 
     def search(self, text=None):
         """See lp.registry.interfaces.product.IProductSet."""
-        # Circular...
-        from lp.registry.model.projectgroup import ProjectGroup
-        conditions = []
         conditions = [Product.active]
         if text:
             conditions.append(
                 SQL("Product.fti @@ ftq(%s) " % sqlvalues(text)))
         result = IStore(Product).find(Product, *conditions)
 
-        def eager_load(rows):
-            product_ids = set(obj.id for obj in rows)
-            if not product_ids:
-                return
-            products = dict((product.id, product) for product in rows)
-            caches = dict((product.id, get_property_cache(product))
-                for product in rows)
-            for cache in caches.values():
-                if not safe_hasattr(cache, 'commercial_subscription'):
-                    cache.commercial_subscription = None
-                if not safe_hasattr(cache, '_cached_licenses'):
-                    cache._cached_licenses = []
-            for subscription in IStore(CommercialSubscription).find(
-                CommercialSubscription,
-                CommercialSubscription.productID.is_in(product_ids)):
-                cache = caches[subscription.productID]
-                cache.commercial_subscription = subscription
-            for license in IStore(ProductLicense).find(
-                ProductLicense,
-                ProductLicense.productID.is_in(product_ids)):
-                cache = caches[license.productID]
-                cache._cached_licenses.append(license.license)
-            for cache in caches.values():
-                cache._cached_licenses = tuple(sorted(cache._cached_licenses))
-            bulk.load_related(ProjectGroup, products.values(), ['projectID'])
-            bulk.load_related(ProductSeries, products.values(),
-                ['development_focusID'])
-            # Only need the objects for canonical_url, no need for validity.
-            bulk.load_related(Person, products.values(),
-                ['_ownerID', 'registrantID', 'bug_supervisorID', 'driverID'])
+        def eager_load(products):
+            return getPrecachedProducts(
+                products, need_licences=True, need_projects=True,
+                role_names=[
+                    '_owner', 'registrant', 'bug_supervisor', 'driver'])
+
         return DecoratedResultSet(result, pre_iter_hook=eager_load)
 
     def search_sqlobject(self, text):

=== modified file 'lib/lp/registry/model/productrelease.py'
--- lib/lp/registry/model/productrelease.py	2012-09-28 06:25:44 +0000
+++ lib/lp/registry/model/productrelease.py	2012-10-05 03:29:36 +0000
@@ -52,6 +52,7 @@
     sqlvalues,
     )
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
+from lp.services.propertycache import cachedproperty
 
 
 class ProductRelease(SQLBase):
@@ -71,10 +72,14 @@
         notNull=True)
     milestone = ForeignKey(dbName='milestone', foreignKey='Milestone')
 
-    files = SQLMultipleJoin(
+    _files = SQLMultipleJoin(
         'ProductReleaseFile', joinColumn='productrelease',
         orderBy='-date_uploaded', prejoins=['productrelease'])
 
+    @cachedproperty
+    def files(self):
+        return self._files
+
     # properties
     @property
     def codename(self):

=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py	2012-09-27 15:28:38 +0000
+++ lib/lp/registry/model/productseries.py	2012-10-05 03:29:36 +0000
@@ -83,6 +83,7 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.propertycache import cachedproperty
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.sorting import sorted_dotted_numbers
 from lp.services.worlddata.model.language import Language
@@ -210,7 +211,7 @@
         """See `HasMilestonesMixin`."""
         return (Milestone.productseries == self)
 
-    @property
+    @cachedproperty
     def releases(self):
         """See `IProductSeries`."""
         store = Store.of(self)

=== modified file 'lib/lp/registry/templates/product-listing-for-review.pt'
--- lib/lp/registry/templates/product-listing-for-review.pt	2012-05-24 21:38:54 +0000
+++ lib/lp/registry/templates/product-listing-for-review.pt	2012-10-05 03:29:36 +0000
@@ -45,7 +45,7 @@
         <dt>Has releases</dt>
         <dd
           tal:attributes="id string:${context/name/fmt:css-id}-releases"
-          tal:content="structure view/latest_release_with_download_files/image:boolean" />
+          tal:content="structure view/has_download_files/image:boolean" />
       </dl>
 
       <dl class="horizontal"