launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05512
lp:~rvb/launchpad/builders-timeout-bug-887078-eager-load-cachedproperty into lp:launchpad
Raphaël Badin has proposed merging lp:~rvb/launchpad/builders-timeout-bug-887078-eager-load-cachedproperty into lp:launchpad with lp:~rvb/launchpad/builders-timeout-bug-887078-eager-load as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #887078 in Launchpad itself: "Builder:+history timeout"
https://bugs.launchpad.net/launchpad/+bug/887078
For more details, see:
https://code.launchpad.net/~rvb/launchpad/builders-timeout-bug-887078-eager-load-cachedproperty/+merge/82198
This branch is a followup on the work done in ~rvb/launchpad/builders-timeout-bug-887078-eager-load. It uses cachedproperty and eager loading to prefetch data in order to avoid issuing lots of queries per source package recipe build displayed on builder:+history. This branch reduces the number of queries per source package recipe build down to one. This one remaining query is the result of calling SourcePackageRecipeBuild.buildqueue_record for each source package recipe displayed on builder:+history. I'm really not sure if we should use the same trick and cache this so I've filed a bug for this and I'll discuss that with Julian when he's back.
= Tests =
(Note that the test now exercises the full page generation — as opposed to the sole preparation done in the view class)
./bin/test -vvc test_builder_views test_build_history_queries_count_view_recipe_builds
= Q/A =
None.
--
https://code.launchpad.net/~rvb/launchpad/builders-timeout-bug-887078-eager-load-cachedproperty/+merge/82198
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/builders-timeout-bug-887078-eager-load-cachedproperty into lp:launchpad.
=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py 2011-06-16 20:12:00 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py 2011-11-14 17:36:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=F0401,W1001
@@ -65,12 +65,21 @@
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.registry.interfaces.distroseries import IDistroSeriesSet
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.stormexpr import Greatest
+from lp.services.propertycache import (
+ cachedproperty,
+ get_property_cache,
+ )
from lp.soyuz.interfaces.archive import IArchiveSet
from lp.soyuz.model.archive import Archive
@@ -158,7 +167,7 @@
name = Unicode(allow_none=True)
description = Unicode(allow_none=True)
- @property
+ @cachedproperty
def _recipe_data(self):
return Store.of(self).find(
SourcePackageRecipeData,
@@ -173,6 +182,20 @@
def base_branch(self):
return self._recipe_data.base_branch
+ @staticmethod
+ def preLoadDataForSourcePackageRecipes(sourcepackagerecipes):
+ caches = dict((spr.id, get_property_cache(spr))
+ for spr in sourcepackagerecipes)
+ spr_datas = load_referencing(
+ SourcePackageRecipeData,
+ sourcepackagerecipes, ['sourcepackage_recipe_id'])
+ for spr_data in spr_datas:
+ cache = caches[spr_data.sourcepackage_recipe_id]
+ cache._recipe_data = spr_data
+ SourcePackageRecipeData.preLoadReferencedBranches(spr_datas)
+ load_related(
+ Branch, spr_datas, ['base_branch_id'])
+
def setRecipeText(self, recipe_text):
parsed = SourcePackageRecipeData.getParsedRecipe(recipe_text)
self._recipe_data.setRecipe(parsed)
=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py 2011-11-14 17:36:35 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py 2011-11-14 17:36:39 +0000
@@ -66,14 +66,10 @@
from lp.code.mail.sourcepackagerecipebuild import (
SourcePackageRecipeBuildMailer,
)
-from lp.code.model.branch import Branch
from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.model.person import Person
-from lp.services.database.bulk import (
- load_referencing,
- load_related,
- )
+from lp.services.database.bulk import load_related
from lp.services.job.model.job import Job
from lp.soyuz.interfaces.archive import CannotUploadToArchive
from lp.soyuz.model.archive import Archive
@@ -305,16 +301,11 @@
from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
package_builds = load_related(
PackageBuild, rows, ['package_build_id'])
- load_related(
- Archive, package_builds, ['archive_id'])
+ archives = load_related(Archive, package_builds, ['archive_id'])
+ load_related(Person, archives, ['ownerID'])
sprs = load_related(
SourcePackageRecipe, rows, ['recipe_id'])
- sprds = load_referencing(
- SourcePackageRecipeData, sprs, ['sourcepackage_recipe_id'])
- load_related(
- Branch, sprds, ['base_branch_id'])
- load_related(
- Person, rows, ['requester_id'])
+ SourcePackageRecipe.preLoadDataForSourcePackageRecipes(sprs)
resultset = Store.of(build_farm_jobs[0]).find(cls,
cls.package_build_id == PackageBuild.id,
PackageBuild.build_farm_job_id.is_in(build_farm_job_ids))
=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py 2010-10-28 15:59:13 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py 2011-11-14 17:36:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=F0401,E1002
@@ -13,6 +13,8 @@
__metaclass__ = type
__all__ = ['SourcePackageRecipeData']
+from itertools import groupby
+
from bzrlib.plugins.builder.recipe import (
BaseRecipeBranch,
MergeInstruction,
@@ -48,6 +50,15 @@
)
from lp.code.interfaces.branchlookup import IBranchLookup
from lp.code.model.branch import Branch
+from lp.services.database.bulk import (
+ load_referencing,
+ load_related,
+ )
+from lp.services.propertycache import (
+ cachedproperty,
+ clear_property_cache,
+ get_property_cache,
+ )
class InstructionType(DBEnumeratedType):
@@ -276,6 +287,7 @@
def setRecipe(self, builder_recipe):
"""Convert the BaseRecipeBranch `builder_recipe` to the db form."""
+ clear_property_cache(self)
if builder_recipe.format > MAX_RECIPE_FORMAT:
raise TooNewRecipeFormat(builder_recipe.format, MAX_RECIPE_FORMAT)
branch_map = self._scanInstructions(builder_recipe)
@@ -306,13 +318,38 @@
self.sourcepackage_recipe = sourcepackage_recipe
self.sourcepackage_recipe_build = sourcepackage_recipe_build
+ @staticmethod
+ def preLoadReferencedBranches(sourcepackagerecipedatas):
+ caches = dict((sprd.id, [sprd, get_property_cache(sprd)])
+ for sprd in sourcepackagerecipedatas)
+ 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'])
+ branch_to_recipe_data = dict([
+ (instr.branch_id, instr.recipe_data_id)
+ for instr in sprd_instructions])
+ for unused, [sprd, cache] in caches.items():
+ cache._referenced_branches = [sprd.base_branch]
+ for recipe_data_id, branches in groupby(
+ sub_branches, lambda branch: branch_to_recipe_data[branch.id]):
+ cache = caches[recipe_data_id][1]
+ cache._referenced_branches.extend(list(branches))
+
def getReferencedBranches(self):
"""Return an iterator of the Branch objects referenced by this recipe.
"""
- yield self.base_branch
+ return self._referenced_branches
+
+ @cachedproperty
+ def _referenced_branches(self):
+ referenced_branches = [self.base_branch]
sub_branches = IStore(self).find(
Branch,
_SourcePackageRecipeDataInstruction.recipe_data == self,
Branch.id == _SourcePackageRecipeDataInstruction.branch_id)
- for branch in sub_branches:
- yield branch
+ referenced_branches.extend(sub_branches)
+ return referenced_branches
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2011-10-18 12:57:33 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2011-11-14 17:36:39 +0000
@@ -55,12 +55,15 @@
SourcePackageRecipeBuild,
SourcePackageRecipeBuildJob,
)
+from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
from lp.code.tests.helpers import recipe_parser_newest_version
from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.database.bulk import load_referencing
from lp.services.job.interfaces.job import (
IJob,
JobStatus,
)
+from lp.services.propertycache import clear_property_cache
from lp.soyuz.enums import ArchivePurpose
from lp.soyuz.interfaces.archive import (
ArchiveDisabled,
@@ -203,18 +206,34 @@
transaction.commit()
self.assertEquals([branch], list(sp_recipe.getReferencedBranches()))
+ def createSourcePackageRecipe(self, number_of_branches=2):
+ branches = []
+ for i in range(number_of_branches):
+ branches.append(self.factory.makeAnyBranch())
+ sp_recipe = self.factory.makeSourcePackageRecipe(branches=branches)
+ transaction.commit()
+ return sp_recipe, branches
+
def test_multiple_branch_links_created(self):
# If a recipe links to more than one branch, getReferencedBranches()
# returns all of them.
- branch1 = self.factory.makeAnyBranch()
- branch2 = self.factory.makeAnyBranch()
- sp_recipe = self.factory.makeSourcePackageRecipe(
- branches=[branch1, branch2])
- transaction.commit()
+ sp_recipe, [branch1, branch2] = self.createSourcePackageRecipe()
self.assertEquals(
sorted([branch1, branch2]),
sorted(sp_recipe.getReferencedBranches()))
+ def test_preLoadReferencedBranches(self):
+ sp_recipe, unused = self.createSourcePackageRecipe()
+ recipe_data = load_referencing(
+ SourcePackageRecipeData,
+ [sp_recipe], ['sourcepackage_recipe_id'])[0]
+ referenced_branches = sp_recipe.getReferencedBranches()
+ clear_property_cache(recipe_data)
+ SourcePackageRecipeData.preLoadReferencedBranches([recipe_data])
+ self.assertEquals(
+ referenced_branches,
+ sorted(sp_recipe.getReferencedBranches()))
+
def test_random_user_cant_edit(self):
# An arbitrary user can't set attributes.
branch1 = self.factory.makeAnyBranch()
=== modified file 'lib/lp/soyuz/browser/tests/test_builder_views.py'
--- lib/lp/soyuz/browser/tests/test_builder_views.py 2011-11-14 17:36:35 +0000
+++ lib/lp/soyuz/browser/tests/test_builder_views.py 2011-11-14 17:36:39 +0000
@@ -104,20 +104,18 @@
# The builder's history view creation (i.e. the call to
# view.setupBuildList) issues a constant number of queries
# when recipe builds are displayed.
- def call_setupBuildList():
- view = create_initialized_view(self.builder, '+history')
- view.setupBuildList()
+ def call_history_render():
+ create_initialized_view(self.builder, '+history').render()
recorder1, recorder2 = self._record_queries_count(
- call_setupBuildList,
+ call_history_render,
self.createRecipeBuildWithBuilder)
- # rvb 2011-11-11: Each build issues 2 new queries.
- # This is because of the way SourcePackageRecipe._recipe_data
- # fetches SourcePackageRecipeData. I've no idea how to prefetch
- # this.
+ # XXX: rvb 2011-11-14 bug=890326: The only query remaining is the
+ # one that results from a call to
+ # sourcepackagerecipebuild.buildqueue_record for each recipe build.
self.assertThat(
recorder2,
- HasQueryCount(Equals(recorder1.count + 2 * self.nb_objects)))
+ HasQueryCount(Equals(recorder1.count + 1 * self.nb_objects)))
def test_build_history_queries_count_binary_package_builds(self):
# Rendering to builder's history issues a constant number of queries
