launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05668
[Merge] lp:~rvb/launchpad/activereviews-bug-893702 into lp:launchpad
Raphaël Badin has proposed merging lp:~rvb/launchpad/activereviews-bug-893702 into lp:launchpad with lp:~rvb/launchpad/activereviews-bug-867941-eagerload2 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #893702 in Launchpad itself: "product:+activereviews times out"
https://bugs.launchpad.net/launchpad/+bug/893702
For more details, see:
https://code.launchpad.net/~rvb/launchpad/activereviews-bug-893702/+merge/83172
This branch reuses the method created while fixing 867941 to eager load the objects related to a list of branch merge proposals for Product:+activereviews.
The real change is the addition of eager_load=True to the call to getMergeProposals in ActiveReviewsView.getProposals.
All the rest is refactoring / cleaning up. I unified the various pre-loading methods to be all static methods and merged the two existing methods to preload objects related to BMPs. I also explicitly added an eager_load argument to the only method in branchcollection.py that was doing that by default previously.
I used store.find instead of load_related/load_referencing when it could save a query.
= Tests =
./bin/test -vvc test_branchmergeproposallisting test_product_activereviews_query_count
= Q/A =
The number of repeated queries on product:+activereviews should be zero.
Here are a few numbers:
qastaging https://code.qastaging.launchpad.net/launchpad/+activereviews => 206 queries
qastaging https://code.qastaging.launchpad.net/openstack/+activereviews => 306 queries
prod https://code.launchpad.net/openobject-addons/+activereviews => 1858+ (timeout), 1243+ (timeout)
The proper QA here is to trigger an oops: https://code.qastaging.launchpad.net/launchpad/+activereviews/++oops++ and then have a look at the repeated queries to make sure all the required objects have been pre loaded.
--
https://code.launchpad.net/~rvb/launchpad/activereviews-bug-893702/+merge/83172
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/activereviews-bug-893702 into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
--- lib/lp/code/browser/branchmergeproposallisting.py 2011-11-22 22:15:11 +0000
+++ lib/lp/code/browser/branchmergeproposallisting.py 2011-11-24 09:15:59 +0000
@@ -286,7 +286,8 @@
collection = collection.visibleByUser(self.user)
proposals = collection.getMergeProposals(
[BranchMergeProposalStatus.CODE_APPROVED,
- BranchMergeProposalStatus.NEEDS_REVIEW, ])
+ BranchMergeProposalStatus.NEEDS_REVIEW, ],
+ eager_load=True)
return proposals
def _getReviewGroup(self, proposal, votes, reviewer):
@@ -447,7 +448,8 @@
proposals = collection.getMergeProposalsForPerson(
self._getReviewer(),
[BranchMergeProposalStatus.CODE_APPROVED,
- BranchMergeProposalStatus.NEEDS_REVIEW])
+ BranchMergeProposalStatus.NEEDS_REVIEW],
+ eager_load=True)
return proposals
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2011-11-22 10:18:22 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2011-11-24 09:15:59 +0000
@@ -393,7 +393,12 @@
layer = LaunchpadFunctionalLayer
- def createBMP(self, reviewer=None, target_branch_owner=None):
+ def setupBMP(self, bmp):
+ self.factory.makePreviewDiff(merge_proposal=bmp)
+ login_person(bmp.source_branch.owner)
+ bmp.requestReview()
+
+ def createUserBMP(self, reviewer=None, target_branch_owner=None):
target_branch = None
if target_branch_owner is not None:
target_branch = self.factory.makePackageBranch(
@@ -401,14 +406,12 @@
bmp = self.factory.makeBranchMergeProposal(
reviewer=reviewer,
target_branch=target_branch)
- self.factory.makePreviewDiff(merge_proposal=bmp)
- login_person(bmp.source_branch.owner)
- bmp.requestReview()
+ self.setupBMP(bmp)
return bmp
- def createBMPsAndRecordQueries(self, number_of_bmps):
+ def createUserBMPsAndRecordQueries(self, number_of_bmps):
# Create {number_of_bmps} branch merge proposals related to a
- # user, render the person's +activereviews page and return the
+ # user, render the person's +activereviews page, and return the
# view and a recorder of the queries generated by this page
# rendering.
user = self.factory.makePerson()
@@ -419,9 +422,9 @@
# - A BMP for which the person is the owner of the target
# branch.
if i % 2 == 0:
- self.createBMP(target_branch_owner=user)
+ self.createUserBMP(target_branch_owner=user)
else:
- self.createBMP(reviewer=user)
+ self.createUserBMP(reviewer=user)
login_person(user)
flush_database_caches()
with StormStatementRecorder() as recorder:
@@ -431,16 +434,50 @@
view.render()
return recorder, view
- def test_activereviews_query_count(self):
+ def test_person_activereviews_query_count(self):
# Note that we keep the number of bmps created small (3 and 7)
# so that the all the per-cached objects will fit into the cache
# used in tests (storm_cache_size: 100).
base_bmps = 3
added_bmps = 4
- recorder1, view1 = self.createBMPsAndRecordQueries(base_bmps)
- self.assertEqual(base_bmps, view1.proposal_count)
- self.addDetail("r1tb", Content(UTF8_TEXT, lambda: [str(recorder1)]))
- recorder2, view2 = self.createBMPsAndRecordQueries(
+ recorder1, view1 = self.createUserBMPsAndRecordQueries(base_bmps)
+ self.assertEqual(base_bmps, view1.proposal_count)
+ self.addDetail("r1tb", Content(UTF8_TEXT, lambda: [str(recorder1)]))
+ recorder2, view2 = self.createUserBMPsAndRecordQueries(
+ base_bmps + added_bmps)
+ self.assertEqual(base_bmps + added_bmps, view2.proposal_count)
+ self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
+ def createProductBMP(self, product):
+ bmp = self.factory.makeBranchMergeProposal(product=product)
+ self.setupBMP(bmp)
+ return bmp
+
+ def createProductBMPsAndRecordQueries(self, number_of_bmps):
+ # Create {number_of_bmps} branch merge proposals related to a
+ # product, render the product's +activereviews page, and return the
+ # view and a recorder of the queries generated by this page
+ # rendering.
+ product = self.factory.makeProduct()
+ for i in xrange(number_of_bmps):
+ self.createProductBMP(product=product)
+ login_person(product.owner)
+ flush_database_caches()
+ with StormStatementRecorder() as recorder:
+ view = create_initialized_view(
+ product, name='+activereviews', rootsite='code',
+ principal=product.owner)
+ view.render()
+ return recorder, view
+
+ def test_product_activereviews_query_count(self):
+ # We keep the number of bmps created small (3 and 7), see above.
+ base_bmps = 3
+ added_bmps = 4
+ recorder1, view1 = self.createProductBMPsAndRecordQueries(base_bmps)
+ self.assertEqual(base_bmps, view1.proposal_count)
+ self.addDetail("r1tb", Content(UTF8_TEXT, lambda: [str(recorder1)]))
+ recorder2, view2 = self.createProductBMPsAndRecordQueries(
base_bmps + added_bmps)
self.assertEqual(base_bmps + added_bmps, view2.proposal_count)
self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-11-23 15:58:34 +0000
+++ lib/lp/code/model/branchcollection.py 2011-11-24 09:15:59 +0000
@@ -64,18 +64,12 @@
from lp.code.model.codeimport import CodeImport
from lp.code.model.codereviewcomment import CodeReviewComment
from lp.code.model.codereviewvote import CodeReviewVoteReference
-from lp.code.model.diff import (
- Diff,
- PreviewDiff,
- )
from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
-from lp.registry.interfaces.person import IPersonSet
from lp.registry.model.distribution import Distribution
from lp.registry.model.distroseries import DistroSeries
from lp.registry.model.person import (
Owner,
Person,
- ValidPersonCache,
)
from lp.registry.model.product import Product
from lp.registry.model.sourcepackagename import SourcePackageName
@@ -223,7 +217,8 @@
return [
With("candidate_branches", SQL("SELECT id from scope_branches"))]
- def _preloadDataForBranches(self, branches):
+ @staticmethod
+ def _preloadDataForBranches(branches):
"""Preload branches cached associated product series and
suite source packages."""
caches = dict((branch.id, get_property_cache(branch))
@@ -239,7 +234,7 @@
# associatedProductSeries
# Imported here to avoid circular import.
from lp.registry.model.productseries import ProductSeries
- for productseries in self.store.find(
+ for productseries in IStore(ProductSeries).find(
ProductSeries,
ProductSeries.branchID.is_in(branch_ids)):
cache = caches[productseries.branchID]
@@ -273,7 +268,7 @@
branch_ids = set(branch.id for branch in rows)
if not branch_ids:
return
- self._preloadDataForBranches(rows)
+ GenericBranchCollection._preloadDataForBranches(rows)
load_related(Product, rows, ['productID'])
# So far have only needed the persons for their canonical_url - no
# need for validity etc in the /branches API call.
@@ -297,46 +292,17 @@
target_branch is not None or
merged_revnos is not None):
return self._naiveGetMergeProposals(statuses, for_branches,
- target_branch, merged_revnos, eager_load)
+ target_branch, merged_revnos, eager_load=eager_load)
else:
# When examining merge proposals in a scope, this is a moderately
# effective set of constrained queries. It is not effective when
# unscoped or when tight constraints on branches are present.
- return self._scopedGetMergeProposals(statuses)
+ return self._scopedGetMergeProposals(
+ statuses, eager_load=eager_load)
def _naiveGetMergeProposals(self, statuses=None, for_branches=None,
- target_branch=None, merged_revnos=None, eager_load=False):
-
- def do_eager_load(rows):
- branch_ids = set()
- person_ids = set()
- diff_ids = set()
- for mp in rows:
- branch_ids.add(mp.target_branchID)
- branch_ids.add(mp.source_branchID)
- person_ids.add(mp.registrantID)
- person_ids.add(mp.merge_reporterID)
- diff_ids.add(mp.preview_diff_id)
- if not branch_ids:
- return
-
- # Pre-load Person and ValidPersonCache.
- list(self.store.find(
- (Person, ValidPersonCache),
- ValidPersonCache.id == Person.id,
- Person.id.is_in(person_ids),
- ))
-
- # Pre-load PreviewDiffs and Diffs.
- list(self.store.find(
- (PreviewDiff, Diff),
- PreviewDiff.id.is_in(diff_ids),
- Diff.id == PreviewDiff.diff_id))
-
- branches = set(
- self.store.find(Branch, Branch.id.is_in(branch_ids)))
- self._preloadDataForBranches(branches)
-
+ target_branch=None, merged_revnos=None,
+ eager_load=False):
Target = ClassAlias(Branch, "target")
extra_tables = list(set(
self._tables.values() + self._asymmetric_tables.values()))
@@ -367,9 +333,11 @@
if not eager_load:
return resultset
else:
- return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
+ return DecoratedResultSet(
+ resultset,
+ pre_iter_hook=BranchMergeProposal._preloadDataForBMPs)
- def _scopedGetMergeProposals(self, statuses):
+ def _scopedGetMergeProposals(self, statuses, eager_load=False):
scope_tables = [Branch] + self._tables.values()
scope_expressions = self._branch_filter_expressions
select = self.store.using(*scope_tables).find(
@@ -392,10 +360,17 @@
if statuses is not None:
expressions.append(
BranchMergeProposal.queue_status.is_in(statuses))
- return self.store.with_(with_expr).using(*tables).find(
+ resultset = self.store.with_(with_expr).using(*tables).find(
BranchMergeProposal, *expressions)
+ if not eager_load:
+ return resultset
+ else:
+ return DecoratedResultSet(
+ resultset,
+ pre_iter_hook=BranchMergeProposal._preloadDataForBMPs)
- def getMergeProposalsForPerson(self, person, status=None):
+ def getMergeProposalsForPerson(self, person, status=None,
+ eager_load=False):
"""See `IBranchCollection`."""
# We want to limit the proposals to those where the source branch is
# limited by the defined collection.
@@ -403,31 +378,12 @@
reviewing = self.getMergeProposalsForReviewer(person, status)
resultset = owned.union(reviewing)
- def do_eager_load(rows):
- # Load the related diffs.
- preview_diffs = load_related(
- PreviewDiff, rows, ['preview_diff_id'])
- PreviewDiff.preloadData(preview_diffs)
- load_related(Diff, preview_diffs, ['diff_id'])
- # Load related branches.
- source_branches = load_related(Branch, rows, ['source_branchID'])
- target_branches = load_related(Branch, rows, ['target_branchID'])
- load_related(Branch, rows, ['prerequisite_branchID'])
- # Cache person's data (registrants of the proposal and
- # owners of the source branches).
- person_ids = set().union(
- (proposal.registrantID for proposal in rows),
- (branch.ownerID for branch in source_branches))
- list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
- person_ids, need_validity=True))
- # Preload the data for source/target branches.
- branches = target_branches + source_branches
- load_related(SourcePackageName, branches, ['sourcepackagenameID'])
- load_related(DistroSeries, branches, ['distroseriesID'])
- self._preloadDataForBranches(branches)
- load_related(Product, branches, ['productID'])
-
- return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
+ if not eager_load:
+ return resultset
+ else:
+ return DecoratedResultSet(
+ resultset,
+ pre_iter_hook=BranchMergeProposal._preloadDataForBMPs)
def getMergeProposalsForReviewer(self, reviewer, status=None):
"""See `IBranchCollection`."""
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2011-09-14 10:19:43 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2011-11-24 09:15:59 +0000
@@ -50,7 +50,10 @@
SQLBase,
sqlvalues,
)
-from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from canonical.launchpad.interfaces.lpstorm import (
+ IMasterStore,
+ IStore,
+ )
from lp.code.enums import (
BranchMergeProposalStatus,
CodeReviewVote,
@@ -88,10 +91,13 @@
)
from lp.registry.interfaces.person import (
IPerson,
+ IPersonSet,
validate_public_person,
)
from lp.registry.interfaces.product import IProduct
from lp.registry.model.person import Person
+from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.services.database.bulk import load_related
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
from lp.services.mail.sendmail import validate_message
@@ -904,6 +910,60 @@
diffs = self.getIncrementalDiffs(ranges)
return [range_ for range_, diff in zip(ranges, diffs) if diff is None]
+ @staticmethod
+ def _preloadDataForBMPs(branch_merge_proposals):
+ # Utility to load the data related to a list of bmps.
+ # Circular imports.
+ from lp.code.model.branch import Branch
+ from lp.code.model.branchcollection import GenericBranchCollection
+ from lp.registry.model.product import Product
+ from lp.registry.model.distroseries import DistroSeries
+
+ branch_ids = set()
+ source_branch_ids = set()
+ person_ids = set()
+ diff_ids = set()
+ for mp in branch_merge_proposals:
+ branch_ids.add(mp.target_branchID)
+ branch_ids.add(mp.prerequisite_branchID)
+ branch_ids.add(mp.source_branchID)
+ source_branch_ids.add(mp.source_branchID)
+ person_ids.add(mp.registrantID)
+ person_ids.add(mp.merge_reporterID)
+ diff_ids.add(mp.preview_diff_id)
+
+ if not branch_ids:
+ return
+
+ store = IStore(BranchMergeProposal)
+
+ # Pre-load PreviewDiffs and Diffs.
+ preview_diffs_and_diffs = list(store.find(
+ (PreviewDiff, Diff),
+ PreviewDiff.id.is_in(diff_ids),
+ Diff.id == PreviewDiff.diff_id))
+ PreviewDiff.preloadData(
+ [preview_diff_and_diff[0] for preview_diff_and_diff
+ in list(preview_diffs_and_diffs)])
+
+ branches = set(
+ store.find(Branch, Branch.id.is_in(branch_ids)))
+ GenericBranchCollection._preloadDataForBranches(branches)
+
+ # Add source branch owners' to the list of pre-loaded persons.
+ for branch in branches:
+ if branch.id in source_branch_ids:
+ person_ids.add(branch.ownerID)
+
+ # Pre-load Person and ValidPersonCache.
+ list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+ person_ids, need_validity=True))
+
+ # Pre-load branches' data.
+ load_related(SourcePackageName, branches, ['sourcepackagenameID'])
+ load_related(DistroSeries, branches, ['distroseriesID'])
+ load_related(Product, branches, ['productID'])
+
class BranchMergeProposalGetter:
"""See `IBranchMergeProposalGetter`."""
=== modified file 'lib/lp/translations/model/translationtemplatesbuild.py'
--- lib/lp/translations/model/translationtemplatesbuild.py 2011-11-15 11:08:01 +0000
+++ lib/lp/translations/model/translationtemplatesbuild.py 2011-11-24 09:15:59 +0000
@@ -125,10 +125,9 @@
Branch, rows, ['branch_id'])
load_related(
Product, branches, ['productID'])
- branch_collection = GenericBranchCollection()
# Preload branches cached associated product series and
# suite source packages for all the related branches.
- branch_collection._preloadDataForBranches(branches)
+ GenericBranchCollection._preloadDataForBranches(branches)
resultset = store.find(
TranslationTemplatesBuild,
