launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05680
[Merge] lp:~rvb/launchpad/activereviews-bug-867941-eagerload3 into lp:launchpad
Raphaël Badin has proposed merging lp:~rvb/launchpad/activereviews-bug-867941-eagerload3 into lp:launchpad with lp:~rvb/launchpad/activereviews-bug-893702 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #867941 in Launchpad itself: "person:+activereviews times out"
https://bugs.launchpad.net/launchpad/+bug/867941
For more details, see:
https://code.launchpad.net/~rvb/launchpad/activereviews-bug-867941-eagerload3/+merge/83389
This branch is (hopefully) the last one in a series aimed at improving the performance of +activereviews. This branch improves the preloading of the chain of stacked on branches. It uses a recursive CTE to fetch in one query all the branches on which the branches to be displayed in {person,product}:activereviews are stacked on.
Two things I'd be happy to have a reviewer's option about:
- since the list of branches provided to preloadVisibleStackedOnBranches is a list of visible branches, is it necessary to make sure that all the returned branches are also visible? I think that If a branch is stacked on a non visible branch then it won't be visible ifself.
- I would be happy to avoid using the name 'column1' (which is the default postgresql name for columns created by VALUES expressions but is not a sql standard) in 'SELECT column1 as id FROM (VALUES %s) AS temp[...]' but I could not find a way to do this. Here is a working query if you are wanting to give a try at modifying it: http://paste.ubuntu.com/749381/.
=Tests=
(changed)
/bin/test -vvc test_branchmergeproposallisting test_product_activereviews_query_count
(added)
/bin/test -vvc test_branchcollection test_preloadVisibleStackedOnBranches_visible_private_branches
/bin/test -vvc test_branchcollection test_preloadVisibleStackedOnBranches_anon_public_branches
/bin/test -vvc test_branchcollection test_preloadVisibleStackedOnBranches_non_anon_public_branches
=Q/A=
https://code.qastaging.launchpad.net/~ubuntu-branches/+activereviews should not issue a single repeated query.
--
https://code.launchpad.net/~rvb/launchpad/activereviews-bug-867941-eagerload3/+merge/83389
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/activereviews-bug-867941-eagerload3 into lp:launchpad.
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2011-11-23 14:36:15 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2011-11-25 15:47:26 +0000
@@ -448,8 +448,17 @@
self.assertEqual(base_bmps + added_bmps, view2.proposal_count)
self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+ def createStackedOnChain(self, product):
+ branch = None
+ for i in xrange(5):
+ branch = self.factory.makeAnyBranch(
+ stacked_on=branch, product=product)
+ return branch
+
def createProductBMP(self, product):
- bmp = self.factory.makeBranchMergeProposal(product=product)
+ target_branch = self.createStackedOnChain(product)
+ bmp = self.factory.makeBranchMergeProposal(
+ product=product, target_branch=target_branch)
self.setupBMP(bmp)
return bmp
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-11-24 12:36:57 +0000
+++ lib/lp/code/model/branchcollection.py 2011-11-25 15:47:26 +0000
@@ -9,6 +9,11 @@
]
from collections import defaultdict
+from functools import partial
+from operator import (
+ attrgetter,
+ itemgetter,
+ )
from lazr.restful.utils import safe_hasattr
from storm.expr import (
@@ -29,6 +34,7 @@
from zope.component import getUtility
from zope.interface import implements
+from canonical.database.sqlbase import quote
from canonical.launchpad.components.decoratedresultset import (
DecoratedResultSet,
)
@@ -218,6 +224,42 @@
With("candidate_branches", SQL("SELECT id from scope_branches"))]
@staticmethod
+ def preloadVisibleStackedOnBranches(branches, user=None):
+ """Preload the chains of stacked on branches related to the given list
+ of branches. Only the branches visible for the given user are
+ preloaded/returned.
+
+ """
+ if len(branches) == 0:
+ return
+ store = IStore(Branch)
+ result = store.execute("""
+ WITH RECURSIVE stacked_on_branches_ids AS (
+ SELECT column1 as id FROM (VALUES %s) AS temp
+ UNION
+ SELECT DISTINCT branch.stacked_on
+ FROM stacked_on_branches_ids, Branch AS branch
+ WHERE
+ branch.id = stacked_on_branches_ids.id AND
+ branch.stacked_on IS NOT NULL
+ )
+ SELECT id from stacked_on_branches_ids
+ """ % ', '.join(
+ ["(%s)" % quote(id)
+ for id in map(attrgetter('id'), branches)]))
+ branch_ids = map(itemgetter(0), result.get_all())
+ if user is None:
+ collection_class = AnonymousBranchCollection
+ else:
+ collection_class = VisibleBranchCollection
+ # Not really sure this is useful: if a given branch is visible by a
+ # user, then I think it means that the whole chain of branches on
+ # which is is stacked on is visible by this user
+ collection = collection_class(
+ user, branch_filter_expressions=[Branch.id.is_in(branch_ids)])
+ return list(collection.getBranches())
+
+ @staticmethod
def preloadDataForBranches(branches):
"""Preload branches cached associated product series and
suite source packages."""
@@ -291,8 +333,9 @@
for_branches is not None or
target_branch is not None or
merged_revnos is not None):
- return self._naiveGetMergeProposals(statuses, for_branches,
- target_branch, merged_revnos, eager_load=eager_load)
+ return self._naiveGetMergeProposals(
+ statuses, for_branches, 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
@@ -333,9 +376,9 @@
if not eager_load:
return resultset
else:
- return DecoratedResultSet(
- resultset,
- pre_iter_hook=BranchMergeProposal.preloadDataForBMPs)
+ loader = partial(
+ BranchMergeProposal.preloadDataForBMPs, user=self._user)
+ return DecoratedResultSet(resultset, pre_iter_hook=loader)
def _scopedGetMergeProposals(self, statuses, eager_load=False):
scope_tables = [Branch] + self._tables.values()
@@ -365,9 +408,9 @@
if not eager_load:
return resultset
else:
- return DecoratedResultSet(
- resultset,
- pre_iter_hook=BranchMergeProposal.preloadDataForBMPs)
+ loader = partial(
+ BranchMergeProposal.preloadDataForBMPs, user=self._user)
+ return DecoratedResultSet(resultset, pre_iter_hook=loader)
def getMergeProposalsForPerson(self, person, status=None,
eager_load=False):
@@ -381,9 +424,9 @@
if not eager_load:
return resultset
else:
- return DecoratedResultSet(
- resultset,
- pre_iter_hook=BranchMergeProposal.preloadDataForBMPs)
+ loader = partial(
+ BranchMergeProposal.preloadDataForBMPs, user=self._user)
+ return DecoratedResultSet(resultset, pre_iter_hook=loader)
def getMergeProposalsForReviewer(self, reviewer, status=None):
"""See `IBranchCollection`."""
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2011-11-24 13:26:16 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2011-11-25 15:47:26 +0000
@@ -911,7 +911,7 @@
return [range_ for range_, diff in zip(ranges, diffs) if diff is None]
@staticmethod
- def preloadDataForBMPs(branch_merge_proposals):
+ def preloadDataForBMPs(branch_merge_proposals, user):
# Utility to load the data related to a list of bmps.
# Circular imports.
from lp.code.model.branch import Branch
@@ -933,9 +933,8 @@
"target_branchID", "prerequisite_branchID",
"source_branchID"))
# The stacked on branches are used to check branch visibility.
- # This is only temporary because we should fetch the whole chain
- # of stacked on branches instead of only the first level.
- load_related(Branch, branches, ["stacked_onID"])
+ GenericBranchCollection.preloadVisibleStackedOnBranches(
+ branches, user)
if len(branches) == 0:
return
@@ -951,8 +950,6 @@
[preview_diff_and_diff[0] for preview_diff_and_diff
in preview_diffs_and_diffs])
- GenericBranchCollection.preloadDataForBranches(branches)
-
# Add source branch owners' to the list of pre-loaded persons.
person_ids.update(
branch.ownerID for branch in branches
@@ -966,6 +963,7 @@
load_related(SourcePackageName, branches, ['sourcepackagenameID'])
load_related(DistroSeries, branches, ['distroseriesID'])
load_related(Product, branches, ['productID'])
+ GenericBranchCollection.preloadDataForBranches(branches)
class BranchMergeProposalGetter:
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2011-10-10 00:03:00 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2011-11-25 15:47:26 +0000
@@ -151,6 +151,52 @@
self.store, [Branch.product == branch.product])
self.assertEqual(1, collection.count())
+ def createBranchChain(self, private, depth, owner=None):
+ branch = None
+ for i in xrange(depth):
+ branch = self.factory.makeAnyBranch(
+ owner=owner, stacked_on=branch, private=private)
+ return branch
+
+ def test_preloadVisibleStackedOnBranches_visible_private_branches(self):
+ person = self.factory.makePerson()
+ branch_number = 2
+ depth = 3
+ # Create private branches person can see.
+ branches = []
+ for i in range(branch_number):
+ branches.append(self.createBranchChain(True, depth, person))
+ with person_logged_in(person):
+ all_branches = (
+ GenericBranchCollection.preloadVisibleStackedOnBranches(
+ branches, person))
+ self.assertEqual(len(all_branches), branch_number * depth)
+
+ def test_preloadVisibleStackedOnBranches_anon_public_branches(self):
+ branch_number = 2
+ depth = 3
+ # Create public branches.
+ branches = []
+ for i in range(branch_number):
+ branches.append(self.createBranchChain(False, depth))
+ all_branches = (
+ GenericBranchCollection.preloadVisibleStackedOnBranches(branches))
+ self.assertEqual(len(all_branches), branch_number * depth)
+
+ def test_preloadVisibleStackedOnBranches_non_anon_public_branches(self):
+ person = self.factory.makePerson()
+ branch_number = 2
+ depth = 3
+ # Create public branches.
+ branches = []
+ for i in range(branch_number):
+ branches.append(self.createBranchChain(False, depth, person))
+ with person_logged_in(person):
+ all_branches = (
+ GenericBranchCollection.preloadVisibleStackedOnBranches(
+ branches, person))
+ self.assertEqual(len(all_branches), branch_number * depth)
+
class TestBranchCollectionFilters(TestCaseWithFactory):
