launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/preload-branch-landing-targets into lp:launchpad.

Commit message:
Preload Branch.landing_targets; improve landing_candidates/landing_targets preloading using BranchMergeProposal.preloadDataForBMPs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/preload-branch-landing-targets/+merge/310651

This adds several more queries to Branch:++branch-pending-merges, but it should be worth it.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/preload-branch-landing-targets into lp:launchpad.

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2016-10-12 23:24:24 +0000
+++ lib/lp/_schema_circular_imports.py	2016-11-11 14:26:47 +0000
@@ -240,7 +240,8 @@
 patch_entry_return_type(IBranch, 'getSubscription', IBranchSubscription)
 patch_collection_property(
     IBranch, '_api_landing_candidates', IBranchMergeProposal)
-patch_collection_property(IBranch, 'landing_targets', IBranchMergeProposal)
+patch_collection_property(
+    IBranch, '_api_landing_targets', IBranchMergeProposal)
 patch_plain_parameter_type(IBranch, 'linkBug', 'bug', IBug)
 patch_plain_parameter_type(
     IBranch, 'linkSpecification', 'spec', ISpecification)

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2016-10-15 13:41:19 +0000
+++ lib/lp/code/browser/branch.py	2016-11-11 14:26:47 +0000
@@ -485,7 +485,8 @@
     @cachedproperty
     def landing_targets(self):
         """Return a filtered list of landing targets."""
-        return latest_proposals_for_each_branch(self.context.landing_targets)
+        targets = self.context.getPrecachedLandingTargets(self.user)
+        return latest_proposals_for_each_branch(targets)
 
     @property
     def latest_landing_candidates(self):

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2016-04-06 10:59:15 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2016-11-11 14:26:47 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for BranchView."""
@@ -561,7 +561,25 @@
         view = create_view(branch, '+index')
         with StormStatementRecorder() as recorder:
             view.landing_candidates
-        self.assertThat(recorder, HasQueryCount(Equals(5)))
+        self.assertThat(recorder, HasQueryCount(Equals(13)))
+
+    def test_query_count_landing_targets(self):
+        product = self.factory.makeProduct()
+        branch = self.factory.makeBranch(product=product)
+        for i in range(10):
+            self.factory.makeBranchMergeProposal(source_branch=branch)
+        stacked = self.factory.makeBranch(product=product)
+        target = self.factory.makeBranch(stacked_on=stacked, product=product)
+        prereq = self.factory.makeBranch(product=product)
+        self.factory.makeBranchMergeProposal(
+            source_branch=branch, target_branch=target,
+            prerequisite_branch=prereq)
+        Store.of(branch).flush()
+        Store.of(branch).invalidate()
+        view = create_view(branch, '+index')
+        with StormStatementRecorder() as recorder:
+            view.landing_targets
+        self.assertThat(recorder, HasQueryCount(Equals(12)))
 
     def test_query_count_subscriber_content(self):
         branch = self.factory.makeBranch()

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2016-10-15 02:28:10 +0000
+++ lib/lp/code/interfaces/branch.py	2016-11-11 14:26:47 +0000
@@ -544,14 +544,18 @@
     # These attributes actually have a value_type of IBranchMergeProposal,
     # but uses Interface to prevent circular imports, and the value_type is
     # set near IBranchMergeProposal.
-    landing_targets = exported(
+    landing_targets = Attribute(
+        'A collection of the merge proposals where this branch is '
+        'the source branch.')
+    _api_landing_targets = exported(
         CollectionField(
             title=_('Landing Targets'),
             description=_(
                 'A collection of the merge proposals where this branch is '
                 'the source branch.'),
             readonly=True,
-            value_type=Reference(Interface)))
+            value_type=Reference(Interface)),
+        exported_as='landing_targets')
     landing_candidates = Attribute(
         'A collection of the merge proposals where this branch is '
         'the target branch.')
@@ -573,6 +577,13 @@
             readonly=True,
             value_type=Reference(Interface)))
 
+    def getPrecachedLandingTargets(user):
+        """Return precached landing targets.
+
+        Target and prerequisite branches are preloaded, along with the
+        related chains of stacked-on branches visible to `user`.
+        """
+
     def getPrecachedLandingCandidates(user):
         """Return precached landing candidates.
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2016-10-15 02:28:10 +0000
+++ lib/lp/code/model/branch.py	2016-11-11 14:26:47 +0000
@@ -9,6 +9,7 @@
     ]
 
 from datetime import datetime
+from functools import partial
 import operator
 
 from bzrlib import urlutils
@@ -473,8 +474,20 @@
     date_created = UtcDateTimeCol(notNull=True, default=DEFAULT)
     date_last_modified = UtcDateTimeCol(notNull=True, default=DEFAULT)
 
-    landing_targets = SQLMultipleJoin(
-        'BranchMergeProposal', joinColumn='source_branch')
+    @property
+    def landing_targets(self):
+        """See `IBranch`."""
+        return Store.of(self).find(
+            BranchMergeProposal, BranchMergeProposal.source_branch == self)
+
+    def getPrecachedLandingTargets(self, user):
+        """See `IBranch`."""
+        loader = partial(BranchMergeProposal.preloadDataForBMPs, user=user)
+        return DecoratedResultSet(self.landing_targets, pre_iter_hook=loader)
+
+    @property
+    def _api_landing_targets(self):
+        return self.getPrecachedLandingTargets(getUtility(ILaunchBag).user)
 
     @property
     def active_landing_targets(self):
@@ -494,17 +507,9 @@
 
     def getPrecachedLandingCandidates(self, user):
         """See `IBranch`."""
-        # Circular import.
-        from lp.code.model.branchcollection import GenericBranchCollection
-
-        def eager_load(rows):
-            branches = bulk.load_related(
-                Branch, rows, ['source_branchID', 'prerequisite_branchID'])
-            GenericBranchCollection.preloadVisibleStackedOnBranches(
-                branches, user)
-
+        loader = partial(BranchMergeProposal.preloadDataForBMPs, user=user)
         return DecoratedResultSet(
-            self.landing_candidates, pre_iter_hook=eager_load)
+            self.landing_candidates, pre_iter_hook=loader)
 
     @property
     def _api_landing_candidates(self):

=== modified file 'lib/lp/code/tests/test_branch_webservice.py'
--- lib/lp/code/tests/test_branch_webservice.py	2015-09-30 00:38:30 +0000
+++ lib/lp/code/tests/test_branch_webservice.py	2016-11-11 14:26:47 +0000
@@ -1,13 +1,10 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
 from lazr.restfulclient.errors import BadRequest
-from testtools.matchers import (
-    Equals,
-    LessThan,
-    )
+from testtools.matchers import LessThan
 from zope.component import getUtility
 from zope.security.management import endInteraction
 from zope.security.proxy import removeSecurityProxy
@@ -23,6 +20,7 @@
     launchpadlib_for,
     login_person,
     logout,
+    person_logged_in,
     record_two_runs,
     run_with_login,
     TestCaseWithFactory,
@@ -37,20 +35,19 @@
     layer = DatabaseFunctionalLayer
 
     def test_landing_candidates_constant_queries(self):
-        target = self.factory.makeProduct()
-        login_person(target.owner)
-        trunk = self.factory.makeBranch(target=target)
-        trunk_url = api_url(trunk)
-        webservice = webservice_for_person(
-            target.owner, permission=OAuthPermission.WRITE_PRIVATE)
-        logout()
+        project = self.factory.makeProduct()
+        with person_logged_in(project.owner):
+            trunk = self.factory.makeBranch(target=project)
+            trunk_url = api_url(trunk)
+            webservice = webservice_for_person(
+                project.owner, permission=OAuthPermission.WRITE_PRIVATE)
 
         def create_mp():
             with admin_logged_in():
                 branch = self.factory.makeBranch(
-                    target=target,
+                    target=project,
                     stacked_on=self.factory.makeBranch(
-                        target=target,
+                        target=project,
                         information_type=InformationType.PRIVATESECURITY),
                     information_type=InformationType.PRIVATESECURITY)
                 self.factory.makeBranchMergeProposal(
@@ -62,11 +59,34 @@
         list_mps()
         recorder1, recorder2 = record_two_runs(list_mps, create_mp, 2)
         self.assertThat(recorder1, HasQueryCount(LessThan(30)))
-        # Each new MP triggers a query for Person, BranchMergeProposal
-        # and PreviewDiff. Eventually they should be batched, but this
-        # at least ensures the situation doesn't get worse.
-        self.assertThat(
-            recorder2, HasQueryCount(Equals(recorder1.count + (2 * 3))))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+    def test_landing_targets_constant_queries(self):
+        project = self.factory.makeProduct()
+        with person_logged_in(project.owner):
+            source = self.factory.makeBranch(target=project)
+            source_url = api_url(source)
+            webservice = webservice_for_person(
+                project.owner, permission=OAuthPermission.WRITE_PRIVATE)
+
+        def create_mp():
+            with admin_logged_in():
+                branch = self.factory.makeBranch(
+                    target=project,
+                    stacked_on=self.factory.makeBranch(
+                        target=project,
+                        information_type=InformationType.PRIVATESECURITY),
+                    information_type=InformationType.PRIVATESECURITY)
+                self.factory.makeBranchMergeProposal(
+                    source_branch=source, target_branch=branch)
+
+        def list_mps():
+            webservice.get(source_url + '/landing_targets')
+
+        list_mps()
+        recorder1, recorder2 = record_two_runs(list_mps, create_mp, 2)
+        self.assertThat(recorder1, HasQueryCount(LessThan(30)))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
 
 
 class TestBranchOperations(TestCaseWithFactory):