Merge ~pappacena/launchpad:gitrepo-pending-merges-tuning into launchpad:master

[Thiago F. Pappacena]

Small improvements in performance and allowing to limit the amount of MPs displayed on repository-pending-merges portlet (on git repository page)

This is not exactly ready to be merged, but I would like some opinions.
I've checked with @profilehooks.profile some methods, and took a look on what could be improved for the repository-pending-merges portlet.

There are two places spending quite a lot of processing time:
1- Fetching and counting MPs to show the /+activereivews link
This part fetches all MPs (and they can be a lot) just to count. It cannot be done on the database directly because we need to use Zope to check for permissions. I'm proposing to limit this count to a certain number, and after that just show "more than <limit> MPs" to the user.

2- List the summary of the most recent MPs for each branch
Again, this cannot be totally filtered on the database because of Zope's permission check. But, apart from limiting, I'm proposing some small improvements on the method getting the latest MP per branch.

On the test side, while I was coding the changes, I've wrote a small test to check the processing time. I'm including a slightly different version of what I've writen before, but I'm not sure if we should include this type of check based on the processing time. Specially because this is dependent on the machine running and it might eventually break under unexpected situations (think of a busy buildbot, for example).

Anyway, I'll probably need to polish a bit this MP, but I would like to have some early feedback on it.

[Colin Watson (cjwatson)]

I think it should be possible to express an equivalent of the launchpad.View permission check on merge proposals directly in a database query. We have a good chunk of the machinery for that already (BranchCollection and GitCollection), and I think we just need to use it more effectively here.

As for the remaining limit stuff, it looks to me as though this needs to be reworked using the batch navigation system; that's normally how we handle rendering large collections.

We normally do avoid wallclock speed tests for the sorts of reasons you suggest. In most cases query count tests (especially tests for how query counts scale with the number of objects involved) are good enough proxies, and they have the great virtue of behaving deterministically. I think that would be the case here.

There's a lot of stuff to get your teeth into here! Let's discuss it at the sprint, since that's so close.

e03cd15... by Thiago F. Pappacena on 2020-01-10

removing wallclock-dependent test

63450a2... by Thiago F. Pappacena on 2020-01-10

fetching less results to check the latests

d6456e3... by Thiago F. Pappacena on 2020-01-10

avoiding mixing MPs not included in the resultset (other status, for example)

af51100... by Thiago F. Pappacena on 2020-01-10

explanation about latest_proposals_for_each_branch

Unmerged commits

af51100... by Thiago F. Pappacena on 2020-01-10

explanation about latest_proposals_for_each_branch

d6456e3... by Thiago F. Pappacena on 2020-01-10

avoiding mixing MPs not included in the resultset (other status, for example)

63450a2... by Thiago F. Pappacena on 2020-01-10

fetching less results to check the latests

e03cd15... by Thiago F. Pappacena on 2020-01-10

removing wallclock-dependent test

ec5b051... by Thiago F. Pappacena on 2020-01-09

limiting data processed to display gitrepository-pending-merges.pt

b422150... by Thiago F. Pappacena on 2020-01-09

improving performance and adding optional limit to latest_proposals_for_each_branch

diff --git a/lib/lp/code/browser/branchmergeproposal.py b/lib/lp/code/browser/branchmergeproposal.py
index a1b41fb..498c637 100644
--- a/lib/lp/code/browser/branchmergeproposal.py
+++ b/lib/lp/code/browser/branchmergeproposal.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Views, navigation and actions for BranchMergeProposals."""
@@ -25,6 +25,7 @@ __all__ = [
     'latest_proposals_for_each_branch',
     ]
 
+from collections import OrderedDict
 from functools import wraps
 import operator
 
@@ -35,6 +36,8 @@ from lazr.restful.interfaces import (
     IWebServiceClientRequest,
     )
 import simplejson
+from storm.expr import Desc, Max, Select
+from storm.store import ResultSet
 from zope.component import (
     adapter,
     getMultiAdapter,
@@ -57,7 +60,10 @@ from zope.schema.vocabulary import (
     SimpleTerm,
     SimpleVocabulary,
     )
-
+from zope.security.proxy import (
+    isinstance as zope_isinstance,
+    removeSecurityProxy
+    )
 from lp import _
 from lp.app.browser.launchpadform import (
     action,
@@ -93,12 +99,14 @@ from lp.code.interfaces.codereviewinlinecomment import (
     )
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.code.interfaces.diff import IPreviewDiff
+from lp.code.model.branchmergeproposal import BranchMergeProposal
 from lp.registry.interfaces.person import IPersonSet
 from lp.services.comments.interfaces.conversation import (
     IComment,
     IConversation,
     )
 from lp.services.config import config
+from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.features import getFeatureFlag
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.librarian.interfaces.client import LibrarianServerError
@@ -127,26 +135,64 @@ from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.menu import NavigationMenu
 
 
-def latest_proposals_for_each_branch(proposals):
+def latest_proposals_for_each_branch(proposals, limit=None):
     """Returns the most recent merge proposals for any particular branch.
 
     Also filters out proposals that the logged in user can't see.
+
+    This method uses 2 different approaches depending on the "proposals":
+        - If it's a ResultSet object, it writes a query and fetches the most
+        recent MPs from resultset directly from the database. So, it's safe
+        to have a reasonably big resultset of BranchMergeProposals
+
+        - If it's a iterable - but not a ResultSet -, this method will sort
+        the MPs [O(log n)], iterate over the sorted list [O(n)] and keep only
+        the most recent ones by date_created.
+
+    In summary, prefer to send ResultSets as parameter
     """
-    targets = {}
-    for proposal in proposals:
-        # Don't show the proposal if the user can't see it.
-        if not check_permission('launchpad.View', proposal):
-            continue
-        # Only show the most recent proposal for any given target.
-        date_created = proposal.date_created
-        target = proposal.merge_target
-
-        if target not in targets or date_created > targets[target][1]:
-            targets[target] = (proposal, date_created)
-
-    return sorted(
-        [proposal for proposal, date_created in targets.itervalues()],
-        key=operator.attrgetter('date_created'), reverse=True)
+    if isinstance(proposals, DecoratedResultSet):
+        proposals = removeSecurityProxy(proposals).get_plain_result_set()
+    if zope_isinstance(proposals, ResultSet):
+        # From the proposal IDs, get the biggest one for each branch
+        proposals_ids = proposals.get_select_expr(BranchMergeProposal.id)
+        most_recents = Select(
+            Max(BranchMergeProposal.id),
+            where=BranchMergeProposal.id.is_in(proposals_ids),
+            group_by=[
+                BranchMergeProposal.target_branchID,
+                BranchMergeProposal.target_git_path,
+                BranchMergeProposal.target_git_commit_sha1]
+        )
+        proposals = proposals.find(
+            BranchMergeProposal.id.is_in(most_recents))
+        proposals = proposals.order_by(Desc(BranchMergeProposal.date_created))
+        # TODO: move this permission check to database
+        return [i for i in proposals if check_permission('launchpad.View', i)]
+    else:
+        # Sort by the most recent first
+        proposals = sorted(
+            [i for i in proposals if check_permission('launchpad.View', i)],
+            key=operator.attrgetter('date_created'),
+            reverse=True)
+        # OrderedDict to keep insertion order (that means: most recent
+        # proposals first)
+        targets = OrderedDict()
+        for proposal in proposals:
+            target = proposal.merge_target
+
+            # If we already inserted some target in the dict, it was for sure
+            # the most recent (since they are sorted). Skip here and avoid
+            # check_permission (since it's a bit more expensive than a hash
+            # lookup), limit check and any other processing.
+            if target in targets:
+                continue
+
+            if limit is not None and len(targets) >= limit:
+                break
+            targets[target] = proposal
+
+        return targets.values()
 
 
 class BranchMergeProposalBreadcrumb(Breadcrumb):
diff --git a/lib/lp/code/browser/gitrepository.py b/lib/lp/code/browser/gitrepository.py
index c05dcc1..97c5da8 100644
--- a/lib/lp/code/browser/gitrepository.py
+++ b/lib/lp/code/browser/gitrepository.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git repository views."""
@@ -364,6 +364,14 @@ class GitRepositoryView(InformationTypePortletMixin, LaunchpadView,
     label = page_title
     show_merge_links = True
 
+    # see `initialize_limits` for feature flags controlling these numbers
+    # Maximum branches to count. None for unlimited
+    landing_candidates_limit = None
+
+    # Maximum MPs to be displayed. None for unlimited
+    landing_targets_limit = None
+
+
     def initialize(self):
         super(GitRepositoryView, self).initialize()
         # Cache permission so that the private team owner can be rendered.  The
@@ -374,6 +382,30 @@ class GitRepositoryView(InformationTypePortletMixin, LaunchpadView,
             precache_permission_for_objects(
                 self.request, "launchpad.LimitedView", authorised_people)
 
+        # None means "not calculated", -1 if above landing_candidates_limit
+        # Any other number is the actual number of MPs
+        self._landing_candidates_count = None
+        self.initialize_limits()
+
+    def get_limit_flag_value(self, flag_name):
+        """Gets the valid limit value from the given feature flag name"""
+        try:
+            flag_candidate_limit = int(getFeatureFlag(flag_name))
+            if flag_candidate_limit <= 0:
+                return None
+            return flag_candidate_limit
+        # int('') or int(None) raises those exceptions
+        except (ValueError, TypeError):
+            return None
+
+    def initialize_limits(self):
+        """Initialize landing_candidates_limit and landing_targets_limit
+        from feature flags (unlimited by default)"""
+        self.landing_candidates_limit = self.get_limit_flag_value(
+            'code.git.show_repository_mps.landing_candidates_limit')
+        self.landing_targets_limit = self.get_limit_flag_value(
+            'code.git.show_repository_mps.landing_targets_limit')
+
     @property
     def git_ssh_url(self):
         """The git+ssh:// URL for this repository, adjusted for this user."""
@@ -418,9 +450,31 @@ class GitRepositoryView(InformationTypePortletMixin, LaunchpadView,
 
     @cachedproperty
     def landing_candidates(self):
+        """Returns up to {self.landing_candidates_limit} merge proposals,
+        and sets the landing_candidates_count_text accordingly
+
+        The way it limits is still not the most efficient (it fetches the
+        MPs, and filter here for check_permission), but it's ok for most
+        real-world cases.
+        Ideally, we should filter this directly on the database"""
+        limit = self.landing_candidates_limit
         candidates = self.context.getPrecachedLandingCandidates(self.user)
-        return [proposal for proposal in candidates
-                if check_permission("launchpad.View", proposal)]
+        all_proposals = []
+        self._landing_candidates_count = None
+        i = 0
+        for proposal in candidates:
+            if not check_permission("launchpad.View", proposal):
+                continue
+            i += 1
+            all_proposals.append(proposal)
+            if limit is not None and i >= limit:
+                # If we don't know the total (and only know that it's above
+                # the limit), let's mark it as -1
+                self._landing_candidates_count = -1
+                break
+        if self._landing_candidates_count is None:
+            self._landing_candidates_count = len(all_proposals)
+        return all_proposals
 
     def _getBranchCountText(self, count):
         """Help to show user friendly text."""
@@ -428,19 +482,24 @@ class GitRepositoryView(InformationTypePortletMixin, LaunchpadView,
             return 'No branches'
         elif count == 1:
             return '1 branch'
+        elif count == -1:
+            return '%s branches or more' % self.landing_candidates_limit
         else:
             return '%s branches' % count
 
     @cachedproperty
     def landing_candidate_count_text(self):
-        return self._getBranchCountText(len(self.landing_candidates))
+        if self._landing_candidates_count is None:
+            self.landing_candidates  # call the property to calculate count
+        return self._getBranchCountText(self._landing_candidates_count)
 
     @cachedproperty
     def landing_targets(self):
         """Return a filtered list of active landing targets."""
         targets = self.context.getPrecachedLandingTargets(
             self.user, only_active=True)
-        return latest_proposals_for_each_branch(targets)
+        return latest_proposals_for_each_branch(
+            targets, limit=self.landing_targets_limit)
 
     @property
     def show_rescan_link(self):
diff --git a/lib/lp/code/browser/tests/test_gitrepository.py b/lib/lp/code/browser/tests/test_gitrepository.py
index a38120c..d100088 100644
--- a/lib/lp/code/browser/tests/test_gitrepository.py
+++ b/lib/lp/code/browser/tests/test_gitrepository.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for GitRepositoryView."""
@@ -8,7 +8,10 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 
 import base64
-from datetime import datetime
+from datetime import (
+    datetime,
+    timedelta,
+    )
 import doctest
 from itertools import chain
 from operator import attrgetter
@@ -24,6 +27,7 @@ from testtools.matchers import (
     DocTestMatches,
     Equals,
     Is,
+    LessThan,
     MatchesDict,
     MatchesListwise,
     MatchesSetwise,
@@ -349,11 +353,65 @@ class TestGitRepositoryView(BrowserTestCase):
             set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
         with FeatureFixture({"code.git.show_repository_mps": "on"}):
             with person_logged_in(target_repository.owner):
-                browser = self.getViewBrowser(
-                    source_repository, user=source_repository.owner)
+                with StormStatementRecorder() as recorder:
+                    browser = self.getViewBrowser(
+                        source_repository, user=source_repository.owner)
+                self.assertThat(recorder, HasQueryCount(LessThan(47)))
                 self.assertIsNotNone(
                     find_tag_by_id(browser.contents, 'landing-targets'))
 
+    def test_view_with_landing_targets_multiple_mps_performance(self):
+        """This is a performance test to make sure the Git repo page with
+        MPs has load time close enough to the load time when we disable the
+        MPs.
+
+        To reduce the likelyhood of this test breaking because of machine
+        circunstances or at the buildbot, the marging is 10%, which should
+        be good enough for this test scenario"""
+        product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
+        target_repository = self.factory.makeGitRepository(target=product)
+        source_repository = self.factory.makeGitRepository(target=product)
+        # creates 200 branches in each repo, and 100 MP
+        for i in range(100):
+            [source_master, source_devel] = self.factory.makeGitRefs(
+                source_repository,
+                paths=["refs/heads/master%s" % i, "refs/heads/devel%s" % i])
+            [target_master, target_devel] = self.factory.makeGitRefs(
+                target_repository,
+                paths=["refs/heads/master%s" % i, "refs/heads/develop%s" % i])
+
+            self.factory.makeBranchMergeProposalForGit(
+                target_ref=source_master,
+                source_ref=source_devel,
+                set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+
+        feature_flags = {
+            "code.git.show_repository_mps": "on",
+            "code.git.show_repository_mps.landing_candidates_limit": "10",
+            "code.git.show_repository_mps.landing_targets_limit": "10"}
+        with FeatureFixture(feature_flags):
+            with person_logged_in(target_repository.owner):
+                with StormStatementRecorder() as recorder:
+                    start_time = datetime.now()
+                    browser = self.getViewBrowser(
+                        source_repository, user=source_repository.owner)
+                    rendering_time_with_mp = datetime.now() - start_time
+
+                self.assertThat(recorder, HasQueryCount(Equals(56)))
+                self.assertIsNotNone(
+                    find_tag_by_id(browser.contents, 'landing-targets'))
+
+                summary_fragments = find_tags_by_class(
+                    browser.contents, "branchmergepropostal-summary-fragment")
+                self.assertEqual(10, len(summary_fragments))
+
+                link_match = soupmatchers.HTMLContains(
+                    soupmatchers.Tag(
+                        "link to branches", "a",
+                        text=re.compile("10 branches or more")),
+                )
+                self.assertThat(browser.contents, link_match)
+
     def test_landing_targets_query_count(self):
         product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
         target_repository = self.factory.makeGitRepository(target=product)
diff --git a/lib/lp/code/templates/branchmergeproposal-summary-fragment.pt b/lib/lp/code/templates/branchmergeproposal-summary-fragment.pt
index 355c0c0..1f4c0fc 100644
--- a/lib/lp/code/templates/branchmergeproposal-summary-fragment.pt
+++ b/lib/lp/code/templates/branchmergeproposal-summary-fragment.pt
@@ -3,7 +3,7 @@
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n">
 
-  <div>
+  <div class="branchmergepropostal-summary-fragment">
     <tal:merge-fragment tal:replace="structure context/@@+link-summary" />
   </div>
   <tal:merge-fragment tal:replace="structure context/@@+vote-summary" />