launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-736008 into lp:launchpad.

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

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-736008/+merge/54820

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.
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-736008/+merge/54820
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-736008 into lp:launchpad.

=== 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-25 08:54:34 +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,7 +274,10 @@
             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))
@@ -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)]