Merge lp:~thumper/launchpad/bmp-index-faster into lp:launchpad

[Tim Penhey]

Cache the bzr_identity of the source branch used when rendering new revisions on merge proposal pages.

Many of the slow merge proposal renderings are due to having many revisions to show, and each revision has a link to the source branch. During my investigations last week I found that every query to bzr_identity on a branch causes two DB queries. The branch wraps the branch used for the rendering of the new revisions as a DecoratedBranch - which has a cached property for the bzr_identity.

Both the DecoratedBranch and DecoratedBug were moved into their own module as they were now both being used in the branch and branchmergeproposal browser modules.

The xx-code-review-comments story tests that the rendering still works.

[Tim Penhey (thumper)]

Oh, there was also some drive by lint fixes in the browser branchmergeproposal module.

[Stuart Bishop (stub)]

I notice neither DecoratedBug nor DecoratedBranch have docstrings, nor do they define an interface making them inscrutable. This is particularly confusing for DecoratedBug which provides self.tasks, self.bugtasks, self.bugtask, self.getBugTask(), self.default_bugtask - some of these are synonyms and some of them (getBugTask() and bugtask) return different things despite their names indicating the are synonyms.

We can let this slide on this branch though since this is moving code to a better location, so a net win. It would be nice to merge the synonyms and document things though if possible.

Otherwise all fine.

review: Approve

[Tim Penhey (thumper)]

On Thu, 17 Jun 2010 17:07:26 you wrote:
> Review: Approve
> I notice neither DecoratedBug nor DecoratedBranch have docstrings, nor do
> they define an interface making them inscrutable. This is particularly
> confusing for DecoratedBug which provides self.tasks, self.bugtasks,
> self.bugtask, self.getBugTask(), self.default_bugtask - some of these are
> synonyms and some of them (getBugTask() and bugtask) return different
> things despite their names indicating the are synonyms.
>
> We can let this slide on this branch though since this is moving code to a
> better location, so a net win. It would be nice to merge the synonyms and
> document things though if possible.
>
> Otherwise all fine.

Thanks.

Did some docstring additions and simplified the bugtasks.

Tim

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2010-06-14 02:37:06 +0000
+++ lib/lp/code/browser/branch.py	2010-06-18 02:09:26 +0000
@@ -25,14 +25,11 @@
     'BranchURL',
     'BranchView',
     'BranchSubscriptionsView',
-    'DecoratedBranch',
-    'DecoratedBug',
     'RegisterBranchMergeProposalView',
     'TryImportAgainView',
     ]
 
 import cgi
-from collections import defaultdict
 from datetime import datetime, timedelta
 
 import pytz
@@ -47,7 +44,6 @@
 from zope.publisher.interfaces import NotFound
 from zope.schema import Bool, Choice, Text
 from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
-from lazr.delegates import delegates
 from lazr.enum import EnumeratedType, Item
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
@@ -79,11 +75,11 @@
 from canonical.widgets.itemswidgets import LaunchpadRadioWidgetWithDescription
 from canonical.widgets.lazrjs import vocabulary_to_choice_edit_items
 
-from lp.bugs.interfaces.bug import IBug
 from lp.bugs.interfaces.bugtask import UNRESOLVED_BUGTASK_STATUSES
 from lp.code.browser.branchref import BranchRef
 from lp.code.browser.branchmergeproposal import (
     latest_proposals_for_each_branch)
+from lp.code.browser.decorations import DecoratedBranch
 from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
 from lp.code.enums import (
     BranchLifecycleStatus, BranchType,
@@ -93,7 +89,7 @@
     CodeImportAlreadyRequested, CodeImportAlreadyRunning,
     CodeImportNotInReviewedState, InvalidBranchMergeProposal)
 from lp.code.interfaces.branch import (
-    BranchCreationForbidden, BranchExists, BzrIdentityMixin, IBranch,
+    BranchCreationForbidden, BranchExists, IBranch,
     user_has_special_branch_access)
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.interfaces.branchtarget import IBranchTarget
@@ -328,108 +324,6 @@
         return Link('+new-recipe', text, enabled=enabled, icon='add')
 
 
-class DecoratedBug:
-    """Provide some additional attributes to a normal bug."""
-    delegates(IBug, 'bug')
-
-    def __init__(self, bug, branch, tasks=None):
-        self.bug = bug
-        self.branch = branch
-        self.tasks = tasks
-        if self.tasks is None:
-            self.tasks = self.bug.bugtasks
-
-    @property
-    def bugtasks(self):
-        return self.tasks
-
-    @property
-    def default_bugtask(self):
-        return self.tasks[0]
-
-    def getBugTask(self, target):
-        # Copied from Bug.getBugTarget to avoid importing.
-        for bugtask in self.bugtasks:
-            if bugtask.target == target:
-                return bugtask
-        return None
-
-    @property
-    def bugtask(self):
-        """Return the bugtask for the branch project, or the default bugtask.
-        """
-        return self.branch.target.getBugTask(self)
-
-
-class DecoratedBranch(BzrIdentityMixin):
-    """Wrap a number of the branch accessors to cache results.
-
-    This avoids repeated db queries.
-    """
-    delegates(IBranch, 'branch')
-
-    def __init__(self, branch):
-        self.branch = branch
-
-    @cachedproperty
-    def linked_bugs(self):
-        bugs = defaultdict(list)
-        for bug, task in self.branch.getLinkedBugsAndTasks():
-            bugs[bug].append(task)
-        return [DecoratedBug(bug, self.branch, tasks)
-                for bug, tasks in bugs.iteritems()
-                if check_permission('launchpad.View', bug)]
-
-    @property
-    def displayname(self):
-        return self.bzr_identity
-
-    @cachedproperty
-    def bzr_identity(self):
-        return super(DecoratedBranch, self).bzr_identity
-
-    @cachedproperty
-    def is_series_branch(self):
-        # True if linked to a product series or suite source package.
-        return (
-            len(self.associated_product_series) > 0 or
-            len(self.suite_source_packages) > 0)
-
-    def associatedProductSeries(self):
-        """Override the IBranch.associatedProductSeries."""
-        return self.associated_product_series
-
-    def associatedSuiteSourcePackages(self):
-        """Override the IBranch.associatedSuiteSourcePackages."""
-        return self.suite_source_packages
-
-    @cachedproperty
-    def associated_product_series(self):
-        return list(self.branch.associatedProductSeries())
-
-    @cachedproperty
-    def suite_source_packages(self):
-        return list(self.branch.associatedSuiteSourcePackages())
-
-    @cachedproperty
-    def upgrade_pending(self):
-        return self.branch.upgrade_pending
-
-    @cachedproperty
-    def subscriptions(self):
-        return list(self.branch.subscriptions)
-
-    def hasSubscription(self, user):
-        for sub in self.subscriptions:
-            if sub.person == user:
-                return True
-        return False
-
-    @cachedproperty
-    def latest_revisions(self):
-        return list(self.branch.latest_revisions())
-
-
 class BranchView(LaunchpadView, FeedsMixin):
 
     __used_for__ = IBranch

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2010-05-16 23:56:51 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2010-06-18 02:09:26 +0000
@@ -70,6 +70,7 @@
 from lp.app.browser.stringformatter import FormattersAPI
 from lp.code.adapters.branch import BranchMergeProposalDelta
 from lp.code.browser.codereviewcomment import CodeReviewDisplayComment
+from lp.code.browser.decorations import DecoratedBranch, DecoratedBug
 from lp.code.enums import (
     BranchMergeProposalStatus, BranchType, CodeReviewNotificationLevel,
     CodeReviewVote)
@@ -212,7 +213,6 @@
     @enabled_with_permission('launchpad.Edit')
     def edit_status(self):
         text = 'Change status'
-        status = self.context.queue_status
         return Link('+edit-status', text, icon='edit')
 
     @enabled_with_permission('launchpad.Edit')
@@ -459,7 +459,6 @@
 
     def _createStatusVocabulary(self):
         # Create the vocabulary that is used for the status widget.
-        curr_status = self.context.queue_status
         possible_next_states = (
             BranchMergeProposalStatus.WORK_IN_PROGRESS,
             BranchMergeProposalStatus.NEEDS_REVIEW,
@@ -590,7 +589,7 @@
         start_date = self.context.date_review_requested
         if start_date is None:
             start_date = self.context.date_created
-        source = self.context.source_branch
+        source = DecoratedBranch(self.context.source_branch)
         resultset = source.getMainlineBranchRevisions(
             start_date, self.revision_end_date, oldest_first=True)
         # Now group by date created.
@@ -663,8 +662,6 @@
     @cachedproperty
     def linked_bugs(self):
         """Return DecoratedBugs linked to the source branch."""
-        # Avoid import loop
-        from lp.code.browser.branch import DecoratedBug
         return [DecoratedBug(bug, self.context.source_branch)
                 for bug in self.context.related_bugs]
 
@@ -839,7 +836,7 @@
     reviewer = copy_field(ICodeReviewVoteReference['reviewer'])
 
     review_type = copy_field(
-        ICodeReviewVoteReference['review_type'], 
+        ICodeReviewVoteReference['review_type'],
         description=u'Lowercase keywords describing the type of review you '
                      'would like to be performed.')
 
@@ -874,8 +871,7 @@
 
     def requestReview(self, candidate, review_type):
         """Request a `review_type` review from `candidate` and email them."""
-        vote_reference = self.context.nominateReviewer(
-            candidate, self.user, review_type)
+        self.context.nominateReviewer(candidate, self.user, review_type)
 
     @action('Request Review', name='review')
     @notify
@@ -1334,7 +1330,7 @@
     vote = copy_field(ICodeReviewComment['vote'], required=True)
 
     review_type = copy_field(
-        ICodeReviewVoteReference['review_type'], 
+        ICodeReviewVoteReference['review_type'],
         description=u'Lowercase keywords describing the type of review you '
                      'are performing.')
 
@@ -1428,7 +1424,7 @@
                 # team.
                 vote_ref.claimReview(self.user)
 
-        comment = self.context.createComment(
+        self.context.createComment(
             self.user, subject=None, content=data['comment'],
             vote=data['vote'], review_type=review_type)
 

=== added file 'lib/lp/code/browser/decorations.py'
--- lib/lp/code/browser/decorations.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/decorations.py	2010-06-18 02:09:26 +0000
@@ -0,0 +1,183 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Decorated model objects used in the browser code."""
+
+__metaclass__ = type
+__all__ = [
+    'DecoratedBranch',
+    'DecoratedBug',
+    ]
+
+
+from collections import defaultdict
+
+from canonical.cachedproperty import cachedproperty
+from canonical.launchpad.webapp.authorization import check_permission
+from lazr.delegates import delegates
+
+from lp.bugs.interfaces.bug import IBug
+from lp.code.interfaces.branch import BzrIdentityMixin, IBranch
+
+
+class DecoratedBug:
+    """Provide some cached attributes to a normal bug.
+
+    We provide cached properties where sensible, and override default bug
+    behaviour where the cached properties can be used to avoid extra database
+    queries.
+    """
+    delegates(IBug, 'bug')
+
+    def __init__(self, bug, branch, tasks=None):
+        self.bug = bug
+        self.branch = branch
+        if tasks is None:
+            tasks = self.bug.bugtasks
+        self.tasks = tasks
+
+    @property
+    def bugtasks(self):
+        """This needs to be a property rather than an attribute.
+
+        If we try to assign to self.bugtasks, the lazr.delegates things we are
+        trying to assign to the property of the bug.
+        """
+        return self.tasks
+
+    @property
+    def default_bugtask(self):
+        """Use the first bugtask.
+
+        Use the cached tasks as calling default_bugtask on the bug object
+        causes a DB query.
+        """
+        return self.bugtasks[0]
+
+    def getBugTask(self, target):
+        """Get the bugtask for a specific target.
+
+        This method is overridden rather than letting it fall through to the
+        underlying bug as the underlying implementation gets the bugtasks from
+        self, which would in that case be the normal bug model object, which
+        would then hit the database to get the tasks.
+        """
+        # Copied from Bug.getBugTarget to avoid importing.
+        for bugtask in self.bugtasks:
+            if bugtask.target == target:
+                return bugtask
+        return None
+
+    @property
+    def bugtask(self):
+        """Return the bugtask for the branch project, or the default bugtask.
+
+        This method defers the identitication of the appropriate task to the
+        branch target.
+        """
+        return self.branch.target.getBugTask(self)
+
+
+class DecoratedBranch(BzrIdentityMixin):
+    """Wrap a number of the branch accessors to cache results.
+
+    This avoids repeated db queries.
+    """
+    delegates(IBranch, 'branch')
+
+    def __init__(self, branch):
+        self.branch = branch
+
+    @cachedproperty
+    def linked_bugs(self):
+        """Override the default behaviour of the branch object.
+
+        The default behaviour is just to get the bugs.  We want to display the
+        tasks however, and to avoid the extra database queries to get the
+        tasks, we get them all at once, and provide decorated bugs (that have
+        their tasks cached).
+        """
+        bugs = defaultdict(list)
+        for bug, task in self.branch.getLinkedBugsAndTasks():
+            bugs[bug].append(task)
+        return [DecoratedBug(bug, self.branch, tasks)
+                for bug, tasks in bugs.iteritems()
+                if check_permission('launchpad.View', bug)]
+
+    @property
+    def displayname(self):
+        """Override the default model property.
+
+        If left to the underlying model, it would call the bzr_identity on the
+        underlying branch rather than the cached bzr_identity on the decorated
+        branch.  And that would cause two database queries.
+        """
+        return self.bzr_identity
+
+    @cachedproperty
+    def bzr_identity(self):
+        """Cache the result of the bzr identity.
+
+        The property is defined in the bzrIdentityMixin class.  This uses the
+        associatedProductSeries and associatedSuiteSourcePackages methods.
+        """
+        return super(DecoratedBranch, self).bzr_identity
+
+    @cachedproperty
+    def is_series_branch(self):
+        """A simple property to see if there are any series links."""
+        # True if linked to a product series or suite source package.
+        return (
+            len(self.associated_product_series) > 0 or
+            len(self.suite_source_packages) > 0)
+
+    def associatedProductSeries(self):
+        """Override the IBranch.associatedProductSeries."""
+        return self.associated_product_series
+
+    def associatedSuiteSourcePackages(self):
+        """Override the IBranch.associatedSuiteSourcePackages."""
+        return self.suite_source_packages
+
+    @cachedproperty
+    def associated_product_series(self):
+        """Cache the realized product series links."""
+        return list(self.branch.associatedProductSeries())
+
+    @cachedproperty
+    def suite_source_packages(self):
+        """Cache the realized suite source package links."""
+        return list(self.branch.associatedSuiteSourcePackages())
+
+    @cachedproperty
+    def upgrade_pending(self):
+        """Cache the result as the property hits the database."""
+        return self.branch.upgrade_pending
+
+    @cachedproperty
+    def subscriptions(self):
+        """Cache the realized branch subscription objects."""
+        return list(self.branch.subscriptions)
+
+    def hasSubscription(self, user):
+        """Override the default branch implementation.
+
+        The default implementation hits the database.  Since we have a full
+        list of subscribers anyway, a simple check over the list is
+        sufficient.
+        """
+        for sub in self.subscriptions:
+            if sub.person == user:
+                return True
+        return False
+
+    @cachedproperty
+    def latest_revisions(self):
+        """Cache the query result.
+
+        When a tal:repeat is used, the method is called twice.  Firstly to
+        check that there is something to iterate over, and secondly for the
+        actual iteration.  Without the cached property, the database is hit
+        twice.
+        """
+        return list(self.branch.latest_revisions())