launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug724025 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #724025 in Launchpad itself: "BugTask:+index timeout due to high cpu time rendering many bug tasks in bug 230350"
  https://bugs.launchpad.net/launchpad/+bug/724025

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug724025/+merge/73432

This tries again to implement some low-hanging optimizations when a bug has many branches.

The main idea of the code was already approved.

https://code.launchpad.net/~gary/launchpad/bug724025/+merge/72776

However, that version was qa-bad: it turned out that when a bug didn't have any branches at all, my optimization was getting *all merge proposals* in the system.  OOPS, as it were.

To fix this, I made two changes.  In the bugtask code I had added before, if there are no branches, I don't bother to try and call getMergeProposals.  This alone would still leave a trap for others, though, so I also made getMergeProposals be more careful: if you pass None for branches or revisions, that means no restriction; but if you pass an empty collection, that means you don't get any results.  IOW, if you want merge proposals that are associated with an empty set of branches, that results in an empty set of merge proposals.

Thank you
-- 
https://code.launchpad.net/~gary/launchpad/bug724025/+merge/73432
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug724025 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-08-30 16:43:16 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-08-30 19:47:25 +0000
@@ -247,6 +247,7 @@
 from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
 from lp.bugs.interfaces.cve import ICveSet
 from lp.bugs.interfaces.malone import IMaloneApplication
+from lp.code.interfaces.branchcollection import IAllBranches
 from lp.registry.interfaces.distribution import (
     IDistribution,
     IDistributionSet,
@@ -882,10 +883,18 @@
     @cachedproperty
     def linked_branches(self):
         """Filter out the bug_branch links to non-visible private branches."""
-        linked_branches = []
-        for linked_branch in self.context.bug.linked_branches:
-            if check_permission('launchpad.View', linked_branch.branch):
-                linked_branches.append(linked_branch)
+        linked_branches = list(
+            self.context.bug.getVisibleLinkedBranches(
+                self.user, eager_load=True))
+        # This is an optimization for when we look at the merge proposals.
+        # Note that, like all of these sorts of Storm cache optimizations, it
+        # only helps if [launchpad] storm_cache_size in launchpad-lazr.conf is
+        # pretty big--and as of this writing, it isn't for developer
+        # instances.
+        if linked_branches:
+            list(getUtility(IAllBranches).getMergeProposals(
+                for_branches=[link.branch for link in linked_branches],
+                eager_load=True))
         return linked_branches
 
     @property

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-08-30 16:43:16 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-08-30 19:47:25 +0000
@@ -124,6 +124,51 @@
                 self.factory.makeBugAttachment(bug=task.bug)
         self.assertThat(task, browses_under_limit)
 
+    def test_rendered_query_counts_reduced_with_branches(self):
+        f = self.factory
+        owner = f.makePerson()
+        ds = f.makeDistroSeries()
+        sourcepackagenames = [
+            f.makeSourcePackageName('testsourcepackagename%d' % i)
+            for i in range(10)]
+        sourcepackages = [
+            f.makeSourcePackage(
+                sourcepackagename=name, distroseries=ds, publish=True)
+            for name in sourcepackagenames]
+        bug = f.makeBug()
+        bugtasks = []
+        for sp in sourcepackages:
+            bugtask = f.makeBugTask(bug=bug, owner=owner, target=sp)
+            bugtasks.append(bugtask)
+        task = bugtasks[0]
+        url = canonical_url(task)
+        recorder = QueryCollector()
+        recorder.register()
+        self.addCleanup(recorder.unregister)
+        self.invalidate_caches(task)
+        self.getUserBrowser(url, owner)
+        # At least 20 of these should be removed.
+        self.assertThat(recorder, HasQueryCount(LessThan(100)))
+        count_with_no_branches = recorder.count
+        self.invalidate_caches(task)
+        with person_logged_in(owner):
+            for sp in sourcepackages:
+                target_branch = f.makePackageBranch(
+                    sourcepackage=sp, owner=owner)
+                source_branch = f.makeBranchTargetBranch(
+                    target_branch.target, owner=owner)
+                bug.linkBranch(source_branch, owner)
+                f.makeBranchMergeProposal(
+                    target_branch=target_branch,
+                    registrant=owner,
+                    source_branch=source_branch)
+        self.getUserBrowser(url, owner)
+        # Ideally this should be much fewer, but this tries to keep a win of
+        # removing more than half of these.
+        self.assertThat(recorder, HasQueryCount(
+            LessThan(count_with_no_branches + 45),
+            ))
+
     def test_interesting_activity(self):
         # The interesting_activity property returns a tuple of interesting
         # `BugActivityItem`s.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-08-30 16:43:16 +0000
+++ lib/lp/bugs/model/bug.py	2011-08-30 19:47:25 +0000
@@ -1295,11 +1295,11 @@
             notify(ObjectDeletedEvent(bug_branch, user=user))
             bug_branch.destroySelf()
 
-    def getVisibleLinkedBranches(self, user):
+    def getVisibleLinkedBranches(self, user, eager_load=False):
         """Return all the branches linked to the bug that `user` can see."""
         all_branches = getUtility(IAllBranches)
         linked_branches = list(all_branches.visibleByUser(
-            user).linkedToBugs([self]).getBranches())
+            user).linkedToBugs([self]).getBranches(eager_load=eager_load))
         if len(linked_branches) == 0:
             return EmptyResultSet()
         else:

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-08-29 07:55:48 +0000
+++ lib/lp/code/model/branchcollection.py	2011-08-30 19:47:25 +0000
@@ -24,6 +24,7 @@
     With,
     )
 from storm.info import ClassAlias
+from storm.store import EmptyResultSet
 from zope.component import getUtility
 from zope.interface import implements
 
@@ -280,8 +281,12 @@
                           target_branch=None, merged_revnos=None,
                           eager_load=False):
         """See `IBranchCollection`."""
-        if (self._asymmetric_filter_expressions or for_branches or
-            target_branch or merged_revnos):
+        if (self._asymmetric_filter_expressions or for_branches is not None or
+            target_branch is not None or merged_revnos is not None):
+            if ((for_branches is not None and not for_branches) or 
+                (merged_revnos is not None and not merged_revnos)):
+                # Make a shortcut.
+                return EmptyResultSet()
             return self._naiveGetMergeProposals(statuses, for_branches,
                 target_branch, merged_revnos, eager_load)
         else:

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2011-08-29 07:55:48 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2011-08-30 19:47:25 +0000
@@ -8,6 +8,7 @@
 from datetime import datetime
 
 import pytz
+from storm.store import EmptyResultSet
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -742,6 +743,28 @@
         proposals = self.all_branches.getMergeProposals()
         self.assertEqual([], list(proposals))
 
+    def test_empty_branches_shortcut(self):
+        # If you explicitly pass an empty collection of branches,
+        # the method shortcuts and gives you an empty result set.  In this
+        # way, for_branches=None (the default) has a very different behavior
+        # than for_branches=[]: the first is no restriction, while the second
+        # excludes everything.
+        mp = self.factory.makeBranchMergeProposal()
+        proposals = self.all_branches.getMergeProposals(for_branches=[])
+        self.assertEqual([], list(proposals))
+        self.assertIsInstance(proposals, EmptyResultSet)
+
+    def test_empty_revisions_shortcut(self):
+        # If you explicitly pass an empty collection of revision numbers,
+        # the method shortcuts and gives you an empty result set.  In this
+        # way, merged_revnos=None (the default) has a very different behavior
+        # than merged_revnos=[]: the first is no restriction, while the second
+        # excludes everything.
+        mp = self.factory.makeBranchMergeProposal()
+        proposals = self.all_branches.getMergeProposals(merged_revnos=[])
+        self.assertEqual([], list(proposals))
+        self.assertIsInstance(proposals, EmptyResultSet)
+
     def test_some_branch_merge_proposals(self):
         mp = self.factory.makeBranchMergeProposal()
         proposals = self.all_branches.getMergeProposals()