Merge lp:~lifeless/launchpad/bug-736008 into lp:launchpad

[Robert Collins]

[r=adeuring][bug=736008] Change to a constrained join rather than a nested query for source branch select in branchmergeproposal lookups.

The query used to determine BranchMergeProposals used differing nested subqueries including one with a product constraint; these perform poorly (because the planner chose a noddy plan), so have changed to a constrained join + identical subqueries (removing one level of nesting) and improving cold cache performance by 10 times for projects with no private branches, and similarly for those that do, except that the query to determine visibility still has some overheads implicit in the design - we may need to refactor further in future.

[Abel Deuring (adeuring)]

review: Approve (code)

[Robert Collins (lifeless)]

I fluffed it and missed this change:
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-03-26 00:50:21 +0000
+++ lib/lp/code/model/branchcollection.py 2011-03-26 19:24:19 +0000
@@ -230,13 +230,8 @@
                 *self._branch_filter_expressions)),
             Join(Target, Target.id==BranchMergeProposal.target_branchID)
             ]
- expressions = []
- source_visible = self._getBranchVisibilityExpression()
- if source_visible:
- expressions.append(BranchMergeProposal.source_branchID.is_in(
- Select(Branch.id, source_visible)))
- expressions.append(BranchMergeProposal.target_branchID.is_in(
- Select(Target.id, self._getBranchVisibilityExpression(Target))))
+ expressions = self._getBranchVisibilityExpression()
+ expressions.extend(self._getBranchVisibilityExpression(Target))
         if for_branches is not None:
             branch_ids = [branch.id for branch in for_branches]
             expressions.append(

The whole point of all the other changes in the branch was to be able to do this :(. I'm self reviewing this incremental step and landing.

review: Approve

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-03-23 16:28:51 +0000
+++ lib/lp/code/model/branchcollection.py	2011-03-26 00:51:33 +0000
@@ -21,6 +21,7 @@
     Select,
     Union,
     )
+from storm.info import ClassAlias
 from zope.component import getUtility
 from zope.interface import implements
 
@@ -161,7 +162,12 @@
 
     def _getBranchExpressions(self):
         """Return the where expressions for this collection."""
-        return self._branch_filter_expressions
+        return (self._branch_filter_expressions
+            + self._getBranchVisibilityExpression())
+
+    def _getBranchVisibilityExpression(self, branch_class=None):
+        """Return the where clauses for visibility."""
+        return []
 
     def getBranches(self, eager_load=False):
         """See `IBranchCollection`."""
@@ -217,10 +223,20 @@
     def getMergeProposals(self, statuses=None, for_branches=None,
                           target_branch=None, merged_revnos=None):
         """See `IBranchCollection`."""
-        expressions = [
-            BranchMergeProposal.source_branchID.is_in(
-                self._getBranchIdQuery()),
+        Target = ClassAlias(Branch, "target")
+        tables = [Branch] + self._tables.values() + [
+            Join(BranchMergeProposal, And(
+                Branch.id==BranchMergeProposal.source_branchID,
+                *self._branch_filter_expressions)),
+            Join(Target, Target.id==BranchMergeProposal.target_branchID)
             ]
+        expressions = []
+        source_visible = self._getBranchVisibilityExpression()
+        if source_visible:
+            expressions.append(BranchMergeProposal.source_branchID.is_in(
+                Select(Branch.id, source_visible)))
+            expressions.append(BranchMergeProposal.target_branchID.is_in(
+                Select(Target.id, self._getBranchVisibilityExpression(Target))))
         if for_branches is not None:
             branch_ids = [branch.id for branch in for_branches]
             expressions.append(
@@ -231,18 +247,10 @@
         if merged_revnos is not None:
             expressions.append(
                 BranchMergeProposal.merged_revno.is_in(merged_revnos))
-        expressions.extend(self._getExtraMergeProposalExpressions())
         if statuses is not None:
             expressions.append(
                 BranchMergeProposal.queue_status.is_in(statuses))
-        return self.store.find(BranchMergeProposal, expressions)
-
-    def _getExtraMergeProposalExpressions(self):
-        """Extra storm expressions needed for merge proposal queries.
-
-        Used primarily by the visibility check for target branches.
-        """
-        return []
+        return self.store.using(*tables).find(BranchMergeProposal, *expressions)
 
     def getMergeProposalsForPerson(self, person, status=None):
         """See `IBranchCollection`."""
@@ -266,12 +274,15 @@
             CodeReviewVoteReference.reviewer == reviewer,
             BranchMergeProposal.source_branchID.is_in(
                 self._getBranchIdQuery())]
-        expressions.extend(self._getExtraMergeProposalExpressions())
+        visibility = self._getBranchVisibilityExpression()
+        if visibility:
+            expressions.append(BranchMergeProposal.target_branchID.is_in(
+                Select(Branch.id, visibility)))
         if status is not None:
             expressions.append(
                 BranchMergeProposal.queue_status.is_in(status))
         proposals = self.store.using(*tables).find(
-            BranchMergeProposal, expressions)
+            BranchMergeProposal, *expressions)
         # Apply sorting here as we can't do it in the browser code.  We need
         # to think carefully about the best places to do this, but not here
         # nor now.
@@ -557,16 +568,10 @@
             store=store,
             branch_filter_expressions=list(branch_filter_expressions),
             tables=tables, exclude_from_search=exclude_from_search)
-        self._branch_filter_expressions.append(Branch.private == False)
-
-    def _getExtraMergeProposalExpressions(self):
-        """Extra storm expressions needed for merge proposal queries.
-
-        Used primarily by the visibility check for target branches.
-        """
-        return [
-            BranchMergeProposal.target_branchID.is_in(
-                Select(Branch.id, Branch.private == False))]
+
+    def _getBranchVisibilityExpression(self, branch_class=Branch):
+        """Return the where clauses for visibility."""
+        return [branch_class.private == False]
 
 
 class VisibleBranchCollection(GenericBranchCollection):
@@ -635,17 +640,21 @@
                        Branch.private == True)))
         return private_branches
 
-    def _getBranchExpressions(self):
-        """Return the where expressions for this collection."""
-        public_branches = Branch.private == False
+    def _getBranchVisibilityExpression(self, branch_class=Branch):
+        """Return the where clauses for visibility.
+        
+        :param branch_class: The Branch class to use - permits using
+            ClassAliases.
+        """
+        public_branches = branch_class.private == False
         if self._private_branch_ids is None:
             # Public only.
-            return self._branch_filter_expressions + [public_branches]
+            return [public_branches]
         else:
             public_or_private = Or(
                 public_branches,
-                Branch.id.is_in(self._private_branch_ids))
-            return self._branch_filter_expressions + [public_or_private]
+                branch_class.id.is_in(self._private_branch_ids))
+            return [public_or_private]
 
     def visibleByUser(self, person):
         """See `IBranchCollection`."""
@@ -654,19 +663,3 @@
         raise InvalidFilter(
             "Cannot filter for branches visible by user %r, already "
             "filtering for %r" % (person, self._user))
-
-    def _getExtraMergeProposalExpressions(self):
-        """Extra storm expressions needed for merge proposal queries.
-
-        Used primarily by the visibility check for target branches.
-        """
-        if self._private_branch_ids is None:
-            # Public only.
-            visible_branches = Select(Branch.id, Branch.private == False)
-        else:
-            visible_branches = Select(
-                Branch.id,
-                Or(Branch.private == False,
-                   Branch.id.is_in(self._private_branch_ids)))
-        return [
-            BranchMergeProposal.target_branchID.is_in(visible_branches)]