Merge lp:~gary/launchpad/bug724025 into lp:launchpad

[Gary Poster]

[incr] [r=lifeless][bug=724025] Reduce queries when you have several branches for a bug.

This branch fixes some low-hanging fruit when you have several branches: significantly reduce the queries in this case. More could be done, but I'm about to move on.

lint is happy.

Thank you

[Robert Collins (lifeless)]

I'm a little uncomfortable with the orthogonal comments about storm cache size - this is a pattern we're using all over the place; if we need to up the storm cache size in dev to match prod, lets just do that. This isn't really an actionable review comment, of course.

review: Approve

[Gary Poster (gary)]

+1 on upping the storm cache on dev, at least to 1000 or so. dev is 100 right now, prod is 10000. I'll make a separate branch for that.

Thanks

[Gary Poster (gary)]

I had to revert this because the branch on which it relied was reverted. Resubmitting after reinstating.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-08-25 20:50:44 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-08-29 18:24:04 +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,17 @@
     @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.
+        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-25 20:50:44 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-08-29 18:24:04 +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-25 20:50:44 +0000
+++ lib/lp/bugs/model/bug.py	2011-08-29 18:24:04 +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: