Merge lp:~rvb/launchpad/activereviews-bug-867941-eagerload2 into lp:launchpad

[Raphaël Badin]

[r=allenap][bug=867941] Eager load objects related to BMPs.

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 =

The goal of this change is to reduce the number of queries issued by person:+activereviews. It does that by eager loading objects related to the bmps displayed… ideally, no repeated statement should be issued after this change has landed. Here is the number of queries currently issued by https://code.launchpad.net/~ubuntu-branches/+activereviews and https://code.qastaging.launchpad.net/~ubuntu-branches/+activereviews
qastaging: [678+, 807+, 1068+, 939+, 1056+, 1140+]
prod: [1405, 849+, 777+, 1034+, 639+, 1382+]
(a '+' next to the number means that the whole page timed out and thus the number of queries is not accurate)

[Robert Collins (lifeless)]

That remaining query just needs a bit more cachedproperty love - see for instance BranchCollection which does the same trick with backreferences elsewhere in the branch space.

[Raphaël Badin (rvb)]

> That remaining query just needs a bit more cachedproperty love - see for
> instance BranchCollection which does the same trick with backreferences
> elsewhere in the branch space.

Thanks for helping me out with this. I did not realize the lookup was caused by a security check.
Since the number of storm references in queries plus the number of places where the field is set is greater than the number of places where the field is read, I've decided to keep branch_merge_proposal as the Reference and add a cached property named _branch_merge_proposal.

[Raphaël Badin (rvb)]

Back to WIP, for consistency, I'll follow lifeless advice to have _branch_merge_proposal be the Reference and branch_merge_proposal the cached property.

[Raphaël Badin (rvb)]

Done.

[Gavin Panella (allenap)]

Looks good.

[1]

+ bmps_preview = dict([(bmp.preview_diff_id, bmp) for bmp in bmps])

No need for the intermediate list here.

review: Approve

[Raphaël Badin (rvb)]

> [1]
>
> + bmps_preview = dict([(bmp.preview_diff_id, bmp) for bmp in bmps])
>
> No need for the intermediate list here.

True. That is something I do very often…

Thanks for the review allenap!

=== 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 16:06:28 +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 16:06:28 +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 16:06:28 +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 != ''
 
-    branch_merge_proposal = Reference(
+    @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 11:26:38 +0000
+++ lib/lp/testing/factory.py	2011-11-22 16:06:28 +0000
@@ -1527,11 +1527,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()