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/82904

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

= Tests =

./bin/test -vvc test_branchmergeproposallisting test_activereviews_query_count

= Q/A =

None.
-- 
https://code.launchpad.net/~rvb/launchpad/activereviews-bug-867941-eagerload2/+merge/82904
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/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-11-17 22:54:10 +0000
+++ lib/canonical/launchpad/security.py	2011-11-22 10:22:27 +0000
@@ -2014,7 +2014,7 @@
     usedfor = IPreviewDiff
 
     def __init__(self, obj):
-        super(PreviewDiffView, self).__init__(obj, obj.branch_merge_proposal)
+        super(PreviewDiffView, self).__init__(obj, obj._branch_merge_proposal)
 
 
 class CodeReviewVoteReferenceEdit(DelegatedAuthorization):

=== 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-22 10:22: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
@@ -431,9 +435,12 @@
         # Note that we keep the number of bmps created small (3 and 7)
         # so that the all the per-cached objects will fit into the cache
         # used in tests (storm_cache_size: 100).
-        recorder1, view1 = self.createBMPsAndRecordQueries(3)
-        self.assertEqual(3, view1.proposal_count)
+        base_bmps = 3
+        added_bmps = 4
+        recorder1, view1 = self.createBMPsAndRecordQueries(base_bmps)
+        self.assertEqual(base_bmps, view1.proposal_count)
         self.addDetail("r1tb", Content(UTF8_TEXT, lambda: [str(recorder1)]))
-        recorder2, view2 = self.createBMPsAndRecordQueries(7)
-        self.assertEqual(7, view2.proposal_count)
+        recorder2, view2 = self.createBMPsAndRecordQueries(
+            base_bmps + added_bmps)
+        self.assertEqual(base_bmps + added_bmps, view2.proposal_count)
         self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-11-19 17:23:00 +0000
+++ lib/lp/code/model/branchcollection.py	2011-11-22 10:22:27 +0000
@@ -441,19 +441,28 @@
             In(BranchMergeProposal.id, result._get_select()))
 
         def do_eager_load(rows):
+            # Load the related diffs.
+            preview_diffs = load_related(
+                PreviewDiff, rows, ['preview_diff_id'])
+            PreviewDiff.preloadData(preview_diffs)
+            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-22 10:22:27 +0000
@@ -50,6 +50,11 @@
     IPreviewDiff,
     )
 from lp.codehosting.bzrutils import read_locked
+from lp.services.database.bulk import load_referencing
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 
 
 class Diff(SQLBase):
@@ -342,10 +347,25 @@
     def has_conflicts(self):
         return self.conflicts is not None and self.conflicts != ''
 
+    @staticmethod
+    def preloadData(preview_diffs):
+        # Circular imports.
+        from lp.code.model.branchmergeproposal import BranchMergeProposal
+        bmps = load_referencing(
+            BranchMergeProposal, preview_diffs, ['preview_diff_id'])
+        bmps_preview = dict([(bmp.preview_diff_id, bmp) for bmp in bmps])
+        for preview_diff in preview_diffs:
+            cache = get_property_cache(preview_diff)
+            cache._branch_merge_proposal = bmps_preview[preview_diff.id]
+
     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-22 00:02:48 +0000
+++ lib/lp/testing/factory.py	2011-11-22 10:22:27 +0000
@@ -1524,11 +1524,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()