launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/launchpad/activereviews-bug-867941-eagerload2 into lp:launchpad with lp:~rvb/launchpad/activereviews-bug-867941-hugequery as a prerequisite.

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-eagerload2/+merge/82862

This branch eager loads more stuff to bring down the number of queries issued by person:+activereviews.

The only trick is the change to lib/lp/code/model/diff.py for which I'd welcome a thorough review.

This line:
        <tal:size replace='proposal/preview_diff/diff_lines_count' condition="proposal/preview_diff"/>
in lib/lp/code/templates/branchmergeproposal-macros.pt was causing one query per MP displayed ("SELECT BMP [...] where merge_diff = {preview_diff.id}."). I figured that this was because of PreviewDiff's back reference to a BMP.  For some reason, materializing the preview_diff object alone would cause this query.  My intention was to add a cachedproperty and populate it manually but the simple change that I made was enough to prevent the query from happening when accessing proposal/preview_diff/diff_lines_count…

= Tests =

./bin/test -vvc test_branchmergeproposallisting test_activereviews_query_count

= Q/A =

None.

 

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

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2011-11-17 09:14:28 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2011-11-21 10:42:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for BranchMergeProposal listing views."""
@@ -16,7 +16,10 @@
 
 from canonical.database.sqlbase import flush_database_caches
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.code.browser.branchmergeproposallisting import (
     ActiveReviewsView,
     BranchMergeProposalListingItem,
@@ -388,16 +391,17 @@
 class PersonActiveReviewsPerformance(TestCaseWithFactory):
     """Test the performance of the person's active reviews page."""
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def createBMP(self, reviewer=None, target_branch_owner=None):
         target_branch = None
         if target_branch_owner is not None:
-            target_branch = self.factory.makeProductBranch(
+            target_branch = self.factory.makePackageBranch(
                 owner=target_branch_owner)
         bmp = self.factory.makeBranchMergeProposal(
             reviewer=reviewer,
             target_branch=target_branch)
+        self.factory.makePreviewDiff(merge_proposal=bmp)
         login_person(bmp.source_branch.owner)
         bmp.requestReview()
         return bmp

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-11-21 10:42:26 +0000
+++ lib/lp/code/model/branchcollection.py	2011-11-21 10:42:27 +0000
@@ -441,19 +441,27 @@
             In(BranchMergeProposal.id, result._get_select()))
 
         def do_eager_load(rows):
+            # Load the related diffs.
+            preview_diffs = load_related(
+                PreviewDiff, rows, ['preview_diff_id'])
+            load_related(Diff, preview_diffs, ['diff_id'])
+            # Load related branches.
             source_branches = load_related(Branch, rows, ['source_branchID'])
+            target_branches = load_related(Branch, rows, ['target_branchID'])
+            load_related(Branch, rows, ['prerequisite_branchID'])
             # Cache person's data (registrants of the proposal and
-            # owners of the source branches).            
+            # owners of the source branches).
             person_ids = set().union(
                 (proposal.registrantID for proposal in rows),
                 (branch.ownerID for branch in source_branches))
             list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
                 person_ids, need_validity=True))
-            # Load the source/target branches and preload the data for
-            # these branches.
-            target_branches = load_related(Branch, rows, ['target_branchID'])
-            self._preloadDataForBranches(target_branches + source_branches)
-            load_related(Product, target_branches, ['productID'])
+            # Preload the data for source/target branches.
+            branches = target_branches + source_branches
+            load_related(SourcePackageName, branches, ['sourcepackagenameID'])
+            load_related(DistroSeries, branches, ['distroseriesID'])
+            self._preloadDataForBranches(branches)
+            load_related(Product, branches, ['productID'])
 
         return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
 

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2011-09-26 06:30:07 +0000
+++ lib/lp/code/model/diff.py	2011-11-21 10:42:27 +0000
@@ -50,6 +50,7 @@
     IPreviewDiff,
     )
 from lp.codehosting.bzrutils import read_locked
+from lp.services.propertycache import cachedproperty
 
 
 class Diff(SQLBase):
@@ -342,10 +343,14 @@
     def has_conflicts(self):
         return self.conflicts is not None and self.conflicts != ''
 
-    branch_merge_proposal = Reference(
+    _branch_merge_proposal = Reference(
         "PreviewDiff.id", "BranchMergeProposal.preview_diff_id",
         on_remote=True)
 
+    @cachedproperty
+    def branch_merge_proposal(self):
+        return self._branch_merge_proposal
+
     @classmethod
     def fromBranchMergeProposal(cls, bmp):
         """Create a `PreviewDiff` from a `BranchMergeProposal`.

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-11-20 23:37:23 +0000
+++ lib/lp/testing/factory.py	2011-11-21 10:42:27 +0000
@@ -1518,11 +1518,12 @@
         return ProxyFactory(
             Diff.fromFile(StringIO(diff_text), len(diff_text)))
 
-    def makePreviewDiff(self, conflicts=u''):
+    def makePreviewDiff(self, conflicts=u'', merge_proposal=None):
         diff = self.makeDiff()
-        bmp = self.makeBranchMergeProposal()
+        if merge_proposal is None:
+            merge_proposal = self.makeBranchMergeProposal()
         preview_diff = PreviewDiff()
-        preview_diff.branch_merge_proposal = bmp
+        preview_diff.branch_merge_proposal = merge_proposal
         preview_diff.conflicts = conflicts
         preview_diff.diff = diff
         preview_diff.source_revision_id = self.getUniqueUnicode()