launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/recipe-timeouts into lp:launchpad.

Commit message:
Preload data needed for recipe listings.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1425080 in Launchpad itself: "Person:+recipes timeouts"
  https://bugs.launchpad.net/launchpad/+bug/1425080

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/recipe-timeouts/+merge/250750

Preload data needed for recipe listings.

I removed the load_related(Branch, spr_datas, ['base_branch_id']) from SourcePackageRecipe.preLoadDataForSourcePackageRecipes since SourcePackageRecipeData.preLoadReferencedBranches already does the exact same thing.  The changes to SourcePackageRecipeData.preLoadReferencedBranches are based on BranchMergeProposal.preloadDataForBMPs.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/recipe-timeouts into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipelisting.py'
--- lib/lp/code/browser/sourcepackagerecipelisting.py	2011-12-24 17:49:30 +0000
+++ lib/lp/code/browser/sourcepackagerecipelisting.py	2015-02-24 13:55:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Base class view for sourcepackagerecipe listings."""
@@ -13,6 +13,7 @@
     ]
 
 
+from lp.code.browser.decorations import DecoratedBranch
 from lp.services.feeds.browser import FeedsMixin
 from lp.services.webapp import (
     canonical_url,
@@ -57,6 +58,13 @@
 
     branch_enabled = False
 
+    def initialize(self):
+        super(BranchRecipeListingView, self).initialize()
+        # Replace our context with a decorated branch, if it is not already
+        # decorated.
+        if not isinstance(self.context, DecoratedBranch):
+            self.context = DecoratedBranch(self.context)
+
 
 class PersonRecipeListingView(RecipeListingView):
 

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py	2014-06-10 16:13:03 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py	2015-02-24 13:55:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for sourcepackagerecipe listings."""
@@ -6,8 +6,15 @@
 __metaclass__ = type
 
 
-from lp.testing import BrowserTestCase
+from testtools.matchers import Equals
+
+from lp.testing import (
+    BrowserTestCase,
+    person_logged_in,
+    record_two_runs,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 
 
 class TestSourcePackageRecipeListing(BrowserTestCase):
@@ -15,7 +22,7 @@
     layer = DatabaseFunctionalLayer
 
     def test_project_branch_recipe_listing(self):
-        # We can see recipes for the product. We need to create two, since
+        # We can see recipes for the project. We need to create two, since
         # only one will redirect to that recipe.
         branch = self.factory.makeProductBranch()
         recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
@@ -37,3 +44,49 @@
             Source Package Recipes for lp:.*
             Name             Owner       Registered
             spr-name.*       Person-name""", text)
+
+    def test_branch_query_count(self):
+        # The number of queries required to render the list of all recipes
+        # for a branch is constant in the number of owners and recipes.
+        person = self.factory.makePerson()
+        branch = self.factory.makeProductBranch(owner=person)
+
+        def create_recipe():
+            with person_logged_in(person):
+                self.factory.makeSourcePackageRecipe(branches=[branch])
+
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getMainText(branch, "+recipes"), create_recipe, 5)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
+    def test_project_query_count(self):
+        # The number of queries required to render the list of all recipes
+        # for a project is constant in the number of owners, branches, and
+        # recipes.
+        person = self.factory.makePerson()
+        project = self.factory.makeProduct(owner=person)
+
+        def create_recipe():
+            with person_logged_in(person):
+                branch = self.factory.makeProductBranch(product=project)
+                self.factory.makeSourcePackageRecipe(branches=[branch])
+
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getMainText(project, "+recipes"), create_recipe, 5)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
+    def test_person_query_count(self):
+        # The number of queries required to render the list of all recipes
+        # for a person is constant in the number of projects, branches, and
+        # recipes.
+        person = self.factory.makePerson()
+
+        def create_recipe():
+            with person_logged_in(person):
+                branch = self.factory.makeProductBranch(owner=person)
+                self.factory.makeSourcePackageRecipe(
+                    owner=person, branches=[branch])
+
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getMainText(person, "+recipes"), create_recipe, 5)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2015-02-16 13:01:34 +0000
+++ lib/lp/code/model/branch.py	2015-02-24 13:55:06 +0000
@@ -1406,9 +1406,13 @@
     @property
     def recipes(self):
         """See `IHasRecipes`."""
+        from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
         from lp.code.model.sourcepackagerecipedata import (
-            SourcePackageRecipeData)
-        return SourcePackageRecipeData.findRecipes(self)
+            SourcePackageRecipeData,
+            )
+        hook = SourcePackageRecipe.preLoadDataForSourcePackageRecipes
+        return DecoratedResultSet(
+            SourcePackageRecipeData.findRecipes(self), pre_iter_hook=hook)
 
     merge_queue_id = Int(name='merge_queue', allow_none=True)
     merge_queue = Reference(merge_queue_id, 'BranchMergeQueue.id')

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2014-07-09 03:08:57 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2015-02-24 13:55:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation of the `SourcePackageRecipe` content type."""
@@ -12,6 +12,7 @@
     datetime,
     timedelta,
     )
+from operator import attrgetter
 
 from lazr.delegates import delegates
 from pytz import utc
@@ -49,16 +50,13 @@
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildSource,
     )
-from lp.code.model.branch import Branch
 from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
 from lp.code.vocabularies.sourcepackagerecipe import BuildableDistroSeries
+from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distroseries import DistroSeries
-from lp.services.database.bulk import (
-    load_referencing,
-    load_related,
-    )
+from lp.services.database.bulk import load_referencing
 from lp.services.database.constants import (
     DEFAULT,
     UTC_NOW,
@@ -166,14 +164,15 @@
         spr_datas = load_referencing(
             SourcePackageRecipeData,
             sourcepackagerecipes, ['sourcepackage_recipe_id'])
-        # Load the related branches.
-        load_related(Branch, spr_datas, ['base_branch_id'])
         # Store the SourcePackageRecipeData in the sourcepackagerecipes
         # objects.
         for spr_data in spr_datas:
             cache = get_property_cache(spr_data.sourcepackage_recipe)
             cache._recipe_data = spr_data
         SourcePackageRecipeData.preLoadReferencedBranches(spr_datas)
+        owner_ids = set(map(attrgetter('owner_id'), sourcepackagerecipes))
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            owner_ids, need_validity=True))
 
     def setRecipeText(self, recipe_text):
         parsed = SourcePackageRecipeData.getParsedRecipe(recipe_text)

=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py	2013-06-20 05:50:00 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py	2015-02-24 13:55:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation of the recipe storage.
@@ -46,6 +46,8 @@
     )
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.model.branch import Branch
+from lp.registry.model.distroseries import DistroSeries
+from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.services.database.bulk import (
     load_referencing,
     load_related,
@@ -323,14 +325,23 @@
 
     @staticmethod
     def preLoadReferencedBranches(sourcepackagerecipedatas):
+        # Circular imports.
+        from lp.code.model.branchcollection import GenericBranchCollection
+        from lp.registry.model.product import Product
         # Load the related Branch, _SourcePackageRecipeDataInstruction.
-        load_related(
+        base_branches = load_related(
             Branch, sourcepackagerecipedatas, ['base_branch_id'])
         sprd_instructions = load_referencing(
             _SourcePackageRecipeDataInstruction,
             sourcepackagerecipedatas, ['recipe_data_id'])
         sub_branches = load_related(
             Branch, sprd_instructions, ['branch_id'])
+        all_branches = base_branches + sub_branches
+        # Pre-load branches' data.
+        load_related(SourcePackageName, all_branches, ['sourcepackagenameID'])
+        load_related(DistroSeries, all_branches, ['distroseriesID'])
+        load_related(Product, all_branches, ['productID'])
+        GenericBranchCollection.preloadDataForBranches(all_branches)
         # Store the pre-fetched objects on the sourcepackagerecipedatas
         # objects.
         branch_to_recipe_data = dict([

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2015-02-06 15:29:45 +0000
+++ lib/lp/registry/model/person.py	2015-02-24 13:55:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation classes for a Person."""
@@ -3158,10 +3158,11 @@
     def recipes(self):
         """See `IHasRecipes`."""
         from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
-        store = Store.of(self)
-        return store.find(
+        recipes = Store.of(self).find(
             SourcePackageRecipe,
             SourcePackageRecipe.owner == self)
+        hook = SourcePackageRecipe.preLoadDataForSourcePackageRecipes
+        return DecoratedResultSet(recipes, pre_iter_hook=hook)
 
     def canAccess(self, obj, attribute):
         """See `IPerson.`"""

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2015-02-13 17:16:22 +0000
+++ lib/lp/registry/model/product.py	2015-02-24 13:55:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database classes including and related to Product."""
@@ -1520,12 +1520,14 @@
     @property
     def recipes(self):
         """See `IHasRecipes`."""
-        return Store.of(self).find(
+        recipes = Store.of(self).find(
             SourcePackageRecipe,
             SourcePackageRecipe.id ==
                 SourcePackageRecipeData.sourcepackage_recipe_id,
             SourcePackageRecipeData.base_branch == Branch.id,
             Branch.product == self)
+        hook = SourcePackageRecipe.preLoadDataForSourcePackageRecipes
+        return DecoratedResultSet(recipes, pre_iter_hook=hook)
 
     def getBugTaskWeightFunction(self):
         """Provide a weight function to determine optimal bug task.