launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/launchpad/activereviews-bug-867941 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #867941 in Launchpad itself: "+activereviews times out"
  https://bugs.launchpad.net/launchpad/+bug/867941

For more details, see:
https://code.launchpad.net/~rvb/launchpad/activereviews-bug-867941/+merge/82375

This branch addresses one of the problems of code.lp.net/~user/+activereviews: the huge number of queries issued by this page (1000+ for pages like https://code.launchpad.net/~ubuntu-branches/+activereviews).  It does this by eager loading the relevant data (the related branches & users mainly).

= Tests =

./bin/test -vvc test_branchmergeproposallisting test_activereviews_query_count

= Q/A =

None.
-- 
https://code.launchpad.net/~rvb/launchpad/activereviews-bug-867941/+merge/82375
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/activereviews-bug-867941 into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2011-08-26 06:33:22 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2011-11-16 10:04:37 +0000
@@ -8,9 +8,11 @@
 from datetime import datetime
 
 import pytz
+from testtools.matchers import Equals
 import transaction
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.database.sqlbase import flush_database_caches
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.code.browser.branchmergeproposallisting import (
@@ -29,8 +31,10 @@
     login,
     login_person,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.views import create_initialized_view
 
 
@@ -377,3 +381,52 @@
             view = create_initialized_view(
                 branch, name='+activereviews', rootsite='code')
             self.assertEqual([mp], list(view.getProposals()))
+
+
+class PersonActiveReviewsPerformance(TestCaseWithFactory):
+    """Test the performance of the person's active reviews page."""
+
+    layer = DatabaseFunctionalLayer
+
+    def createBMP(self, reviewer=None, target_branch_owner=None):
+        target_branch = None
+        if target_branch_owner is not None:
+            target_branch = self.factory.makeProductBranch(
+                owner=target_branch_owner)
+        bmp = self.factory.makeBranchMergeProposal(
+            reviewer=reviewer,
+            target_branch=target_branch)
+        login_person(bmp.source_branch.owner)
+        bmp.requestReview()
+        return bmp
+
+    def createBMPsAndMeasureActiveReviewsPageRendering(self, number_of_bmps):
+        # Create {number_of_bmps} branch merge proposals related to a
+        # user, render the person's +activereviews page and return a
+        # recorder of the queries generated by this page rendering.
+        user = self.factory.makePerson()
+        for i in xrange(number_of_bmps):
+            # Create one of the two types of BMP which will be displayed
+            # on a person's +activereviews page:
+            # - A BMP for which the person is the reviewer.
+            # - A BMP for which the person is the owner of the target
+            # branch.
+            if i % 2 == 0:
+                self.createBMP(target_branch_owner=user)
+            else:
+                self.createBMP(reviewer=user)
+        transaction.commit()
+        login_person(user)
+        flush_database_caches()
+        with StormStatementRecorder() as recorder:
+            view = create_initialized_view(
+                user, name='+activereviews', rootsite='code',
+                principal=user)
+            view.render()
+        self.assertEqual(number_of_bmps, len(list(view.getProposals())))
+        return recorder
+
+    def test_activereviews_query_count(self):
+        recorder1 = self.createBMPsAndMeasureActiveReviewsPageRendering(3)
+        recorder2 = self.createBMPsAndMeasureActiveReviewsPageRendering(7)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-11-01 08:17:57 +0000
+++ lib/lp/code/model/branchcollection.py	2011-11-16 10:04:37 +0000
@@ -400,7 +400,25 @@
         # limited by the defined collection.
         owned = self.ownedBy(person).getMergeProposals(status)
         reviewing = self.getMergeProposalsForReviewer(person, status)
-        return owned.union(reviewing)
+        resultset = owned.union(reviewing)
+
+        def do_eager_load(rows):
+            # Load the registrants' data.
+            load_related(Person, rows, ['registrantID'])
+            # Cache registrants' info (Person and ValidPersonCache).
+            list(self.store.find(
+                (Person, ValidPersonCache),
+                ValidPersonCache.id == Person.id,
+                Person.id.is_in([proposal.registrantID for proposal in rows]),
+                ))
+            # Load the source/target branches and preload the data for
+            # these branches.
+            source_branches = load_related(Branch, rows, ['source_branchID'])
+            target_branches = load_related(Branch, rows, ['target_branchID'])
+            self._preloadDataForBranches(target_branches + source_branches)
+            load_related(Person, source_branches, ['ownerID'])
+            load_related(Product, target_branches, ['productID'])
+        return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
 
     def getMergeProposalsForReviewer(self, reviewer, status=None):
         """See `IBranchCollection`."""