Launchpad itself

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

  1. bmp-index-faster
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Stuart Bishop
Approved revision: no longer in the source branch.
Merged at revision: 11030
Proposed branch: lp:~thumper/launchpad/bmp-index-faster
Merge into: lp:launchpad
Diff against target: 428 lines (+191/-118)
3 files modified
lib/lp/code/browser/branch.py (+2/-108)
lib/lp/code/browser/branchmergeproposal.py (+6/-10)
lib/lp/code/browser/decorations.py (+183/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/bmp-index-faster
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Review via email: mp+27788@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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

Revision history for this message
Stuart Bishop (stub) wrote :

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
Revision history for this message
Tim Penhey (thumper) wrote :

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

bif.diff
1=== modified file 'lib/lp/code/browser/decorations.py'
2--- lib/lp/code/browser/decorations.py 2010-06-17 01:16:30 +0000
3+++ lib/lp/code/browser/decorations.py 2010-06-17 05:57:00 +0000
4@@ -21,25 +21,38 @@
5
6
7 class DecoratedBug:
8- """Provide some additional attributes to a normal bug."""
9+ """Provide some cached attributes to a normal bug.
10+
11+ We provide cached properties where sensible, and override default bug
12+ behaviour where the cached properties can be used to avoid extra database
13+ queries.
14+ """
15 delegates(IBug, 'bug')
16
17 def __init__(self, bug, branch, tasks=None):
18 self.bug = bug
19 self.branch = branch
20- self.tasks = tasks
21- if self.tasks is None:
22- self.tasks = self.bug.bugtasks
23-
24- @property
25- def bugtasks(self):
26- return self.tasks
27+ if tasks is None:
28+ tasks = self.bug.bugtasks
29+ self.bugtasks = tasks
30
31 @property
32 def default_bugtask(self):
33- return self.tasks[0]
34+ """Use the first bugtask.
35+
36+ Use the cached tasks as calling default_bugtask on the bug object
37+ causes a DB query.
38+ """
39+ return self.bugtasks[0]
40
41 def getBugTask(self, target):
42+ """Get the bugtask for a specific target.
43+
44+ This method is overridden rather than letting it fall through to the
45+ underlying bug as the underlying implementation gets the bugtasks from
46+ self, which would in that case be the normal bug model object, which
47+ would then hit the database to get the tasks.
48+ """
49 # Copied from Bug.getBugTarget to avoid importing.
50 for bugtask in self.bugtasks:
51 if bugtask.target == target:
52@@ -49,6 +62,9 @@
53 @property
54 def bugtask(self):
55 """Return the bugtask for the branch project, or the default bugtask.
56+
57+ This method defers the identitication of the appropriate task to the
58+ branch target.
59 """
60 return self.branch.target.getBugTask(self)
61
62@@ -65,6 +81,13 @@
63
64 @cachedproperty
65 def linked_bugs(self):
66+ """Override the default behaviour of the branch object.
67+
68+ The default behaviour is just to get the bugs. We want to display the
69+ tasks however, and to avoid the extra database queries to get the
70+ tasks, we get them all at once, and provide decorated bugs (that have
71+ their tasks cached).
72+ """
73 bugs = defaultdict(list)
74 for bug, task in self.branch.getLinkedBugsAndTasks():
75 bugs[bug].append(task)
76@@ -74,14 +97,26 @@
77
78 @property
79 def displayname(self):
80+ """Override the default model property.
81+
82+ If left to the underlying model, it would call the bzr_identity on the
83+ underlying branch rather than the cached bzr_identity on the decorated
84+ branch. And that would cause two database queries.
85+ """
86 return self.bzr_identity
87
88 @cachedproperty
89 def bzr_identity(self):
90+ """Cache the result of the bzr identity.
91+
92+ The property is defined in the bzrIdentityMixin class. This uses the
93+ associatedProductSeries and associatedSuiteSourcePackages methods.
94+ """
95 return super(DecoratedBranch, self).bzr_identity
96
97 @cachedproperty
98 def is_series_branch(self):
99+ """A simple property to see if there are any series links."""
100 # True if linked to a product series or suite source package.
101 return (
102 len(self.associated_product_series) > 0 or
103@@ -97,21 +132,31 @@
104
105 @cachedproperty
106 def associated_product_series(self):
107+ """Cache the realized product series links."""
108 return list(self.branch.associatedProductSeries())
109
110 @cachedproperty
111 def suite_source_packages(self):
112+ """Cache the realized suite source package links."""
113 return list(self.branch.associatedSuiteSourcePackages())
114
115 @cachedproperty
116 def upgrade_pending(self):
117+ """Cache the result as the property hits the database."""
118 return self.branch.upgrade_pending
119
120 @cachedproperty
121 def subscriptions(self):
122+ """Cache the realized branch subscription objects."""
123 return list(self.branch.subscriptions)
124
125 def hasSubscription(self, user):
126+ """Override the default branch implementation.
127+
128+ The default implementation hits the database. Since we have a full
129+ list of subscribers anyway, a simple check over the list is
130+ sufficient.
131+ """
132 for sub in self.subscriptions:
133 if sub.person == user:
134 return True
135@@ -119,6 +164,11 @@
136
137 @cachedproperty
138 def latest_revisions(self):
139+ """Cache the query result.
140+
141+ When a tal:repeat is used, the method is called twice. Firstly to
142+ check that there is something to iterate over, and secondly for the
143+ actual iteration. Without the cached property, the database is hit
144+ twice.
145+ """
146 return list(self.branch.latest_revisions())
147-
148-

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/branch.py'
2--- lib/lp/code/browser/branch.py 2010-06-14 02:37:06 +0000
3+++ lib/lp/code/browser/branch.py 2010-06-18 02:09:26 +0000
4@@ -25,14 +25,11 @@
5 'BranchURL',
6 'BranchView',
7 'BranchSubscriptionsView',
8- 'DecoratedBranch',
9- 'DecoratedBug',
10 'RegisterBranchMergeProposalView',
11 'TryImportAgainView',
12 ]
13
14 import cgi
15-from collections import defaultdict
16 from datetime import datetime, timedelta
17
18 import pytz
19@@ -47,7 +44,6 @@
20 from zope.publisher.interfaces import NotFound
21 from zope.schema import Bool, Choice, Text
22 from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
23-from lazr.delegates import delegates
24 from lazr.enum import EnumeratedType, Item
25 from lazr.lifecycle.event import ObjectModifiedEvent
26 from lazr.lifecycle.snapshot import Snapshot
27@@ -79,11 +75,11 @@
28 from canonical.widgets.itemswidgets import LaunchpadRadioWidgetWithDescription
29 from canonical.widgets.lazrjs import vocabulary_to_choice_edit_items
30
31-from lp.bugs.interfaces.bug import IBug
32 from lp.bugs.interfaces.bugtask import UNRESOLVED_BUGTASK_STATUSES
33 from lp.code.browser.branchref import BranchRef
34 from lp.code.browser.branchmergeproposal import (
35 latest_proposals_for_each_branch)
36+from lp.code.browser.decorations import DecoratedBranch
37 from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
38 from lp.code.enums import (
39 BranchLifecycleStatus, BranchType,
40@@ -93,7 +89,7 @@
41 CodeImportAlreadyRequested, CodeImportAlreadyRunning,
42 CodeImportNotInReviewedState, InvalidBranchMergeProposal)
43 from lp.code.interfaces.branch import (
44- BranchCreationForbidden, BranchExists, BzrIdentityMixin, IBranch,
45+ BranchCreationForbidden, BranchExists, IBranch,
46 user_has_special_branch_access)
47 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
48 from lp.code.interfaces.branchtarget import IBranchTarget
49@@ -328,108 +324,6 @@
50 return Link('+new-recipe', text, enabled=enabled, icon='add')
51
52
53-class DecoratedBug:
54- """Provide some additional attributes to a normal bug."""
55- delegates(IBug, 'bug')
56-
57- def __init__(self, bug, branch, tasks=None):
58- self.bug = bug
59- self.branch = branch
60- self.tasks = tasks
61- if self.tasks is None:
62- self.tasks = self.bug.bugtasks
63-
64- @property
65- def bugtasks(self):
66- return self.tasks
67-
68- @property
69- def default_bugtask(self):
70- return self.tasks[0]
71-
72- def getBugTask(self, target):
73- # Copied from Bug.getBugTarget to avoid importing.
74- for bugtask in self.bugtasks:
75- if bugtask.target == target:
76- return bugtask
77- return None
78-
79- @property
80- def bugtask(self):
81- """Return the bugtask for the branch project, or the default bugtask.
82- """
83- return self.branch.target.getBugTask(self)
84-
85-
86-class DecoratedBranch(BzrIdentityMixin):
87- """Wrap a number of the branch accessors to cache results.
88-
89- This avoids repeated db queries.
90- """
91- delegates(IBranch, 'branch')
92-
93- def __init__(self, branch):
94- self.branch = branch
95-
96- @cachedproperty
97- def linked_bugs(self):
98- bugs = defaultdict(list)
99- for bug, task in self.branch.getLinkedBugsAndTasks():
100- bugs[bug].append(task)
101- return [DecoratedBug(bug, self.branch, tasks)
102- for bug, tasks in bugs.iteritems()
103- if check_permission('launchpad.View', bug)]
104-
105- @property
106- def displayname(self):
107- return self.bzr_identity
108-
109- @cachedproperty
110- def bzr_identity(self):
111- return super(DecoratedBranch, self).bzr_identity
112-
113- @cachedproperty
114- def is_series_branch(self):
115- # True if linked to a product series or suite source package.
116- return (
117- len(self.associated_product_series) > 0 or
118- len(self.suite_source_packages) > 0)
119-
120- def associatedProductSeries(self):
121- """Override the IBranch.associatedProductSeries."""
122- return self.associated_product_series
123-
124- def associatedSuiteSourcePackages(self):
125- """Override the IBranch.associatedSuiteSourcePackages."""
126- return self.suite_source_packages
127-
128- @cachedproperty
129- def associated_product_series(self):
130- return list(self.branch.associatedProductSeries())
131-
132- @cachedproperty
133- def suite_source_packages(self):
134- return list(self.branch.associatedSuiteSourcePackages())
135-
136- @cachedproperty
137- def upgrade_pending(self):
138- return self.branch.upgrade_pending
139-
140- @cachedproperty
141- def subscriptions(self):
142- return list(self.branch.subscriptions)
143-
144- def hasSubscription(self, user):
145- for sub in self.subscriptions:
146- if sub.person == user:
147- return True
148- return False
149-
150- @cachedproperty
151- def latest_revisions(self):
152- return list(self.branch.latest_revisions())
153-
154-
155 class BranchView(LaunchpadView, FeedsMixin):
156
157 __used_for__ = IBranch
158
159=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
160--- lib/lp/code/browser/branchmergeproposal.py 2010-05-16 23:56:51 +0000
161+++ lib/lp/code/browser/branchmergeproposal.py 2010-06-18 02:09:26 +0000
162@@ -70,6 +70,7 @@
163 from lp.app.browser.stringformatter import FormattersAPI
164 from lp.code.adapters.branch import BranchMergeProposalDelta
165 from lp.code.browser.codereviewcomment import CodeReviewDisplayComment
166+from lp.code.browser.decorations import DecoratedBranch, DecoratedBug
167 from lp.code.enums import (
168 BranchMergeProposalStatus, BranchType, CodeReviewNotificationLevel,
169 CodeReviewVote)
170@@ -212,7 +213,6 @@
171 @enabled_with_permission('launchpad.Edit')
172 def edit_status(self):
173 text = 'Change status'
174- status = self.context.queue_status
175 return Link('+edit-status', text, icon='edit')
176
177 @enabled_with_permission('launchpad.Edit')
178@@ -459,7 +459,6 @@
179
180 def _createStatusVocabulary(self):
181 # Create the vocabulary that is used for the status widget.
182- curr_status = self.context.queue_status
183 possible_next_states = (
184 BranchMergeProposalStatus.WORK_IN_PROGRESS,
185 BranchMergeProposalStatus.NEEDS_REVIEW,
186@@ -590,7 +589,7 @@
187 start_date = self.context.date_review_requested
188 if start_date is None:
189 start_date = self.context.date_created
190- source = self.context.source_branch
191+ source = DecoratedBranch(self.context.source_branch)
192 resultset = source.getMainlineBranchRevisions(
193 start_date, self.revision_end_date, oldest_first=True)
194 # Now group by date created.
195@@ -663,8 +662,6 @@
196 @cachedproperty
197 def linked_bugs(self):
198 """Return DecoratedBugs linked to the source branch."""
199- # Avoid import loop
200- from lp.code.browser.branch import DecoratedBug
201 return [DecoratedBug(bug, self.context.source_branch)
202 for bug in self.context.related_bugs]
203
204@@ -839,7 +836,7 @@
205 reviewer = copy_field(ICodeReviewVoteReference['reviewer'])
206
207 review_type = copy_field(
208- ICodeReviewVoteReference['review_type'],
209+ ICodeReviewVoteReference['review_type'],
210 description=u'Lowercase keywords describing the type of review you '
211 'would like to be performed.')
212
213@@ -874,8 +871,7 @@
214
215 def requestReview(self, candidate, review_type):
216 """Request a `review_type` review from `candidate` and email them."""
217- vote_reference = self.context.nominateReviewer(
218- candidate, self.user, review_type)
219+ self.context.nominateReviewer(candidate, self.user, review_type)
220
221 @action('Request Review', name='review')
222 @notify
223@@ -1334,7 +1330,7 @@
224 vote = copy_field(ICodeReviewComment['vote'], required=True)
225
226 review_type = copy_field(
227- ICodeReviewVoteReference['review_type'],
228+ ICodeReviewVoteReference['review_type'],
229 description=u'Lowercase keywords describing the type of review you '
230 'are performing.')
231
232@@ -1428,7 +1424,7 @@
233 # team.
234 vote_ref.claimReview(self.user)
235
236- comment = self.context.createComment(
237+ self.context.createComment(
238 self.user, subject=None, content=data['comment'],
239 vote=data['vote'], review_type=review_type)
240
241
242=== added file 'lib/lp/code/browser/decorations.py'
243--- lib/lp/code/browser/decorations.py 1970-01-01 00:00:00 +0000
244+++ lib/lp/code/browser/decorations.py 2010-06-18 02:09:26 +0000
245@@ -0,0 +1,183 @@
246+# Copyright 2010 Canonical Ltd. This software is licensed under the
247+# GNU Affero General Public License version 3 (see the file LICENSE).
248+
249+"""Decorated model objects used in the browser code."""
250+
251+__metaclass__ = type
252+__all__ = [
253+ 'DecoratedBranch',
254+ 'DecoratedBug',
255+ ]
256+
257+
258+from collections import defaultdict
259+
260+from canonical.cachedproperty import cachedproperty
261+from canonical.launchpad.webapp.authorization import check_permission
262+from lazr.delegates import delegates
263+
264+from lp.bugs.interfaces.bug import IBug
265+from lp.code.interfaces.branch import BzrIdentityMixin, IBranch
266+
267+
268+class DecoratedBug:
269+ """Provide some cached attributes to a normal bug.
270+
271+ We provide cached properties where sensible, and override default bug
272+ behaviour where the cached properties can be used to avoid extra database
273+ queries.
274+ """
275+ delegates(IBug, 'bug')
276+
277+ def __init__(self, bug, branch, tasks=None):
278+ self.bug = bug
279+ self.branch = branch
280+ if tasks is None:
281+ tasks = self.bug.bugtasks
282+ self.tasks = tasks
283+
284+ @property
285+ def bugtasks(self):
286+ """This needs to be a property rather than an attribute.
287+
288+ If we try to assign to self.bugtasks, the lazr.delegates things we are
289+ trying to assign to the property of the bug.
290+ """
291+ return self.tasks
292+
293+ @property
294+ def default_bugtask(self):
295+ """Use the first bugtask.
296+
297+ Use the cached tasks as calling default_bugtask on the bug object
298+ causes a DB query.
299+ """
300+ return self.bugtasks[0]
301+
302+ def getBugTask(self, target):
303+ """Get the bugtask for a specific target.
304+
305+ This method is overridden rather than letting it fall through to the
306+ underlying bug as the underlying implementation gets the bugtasks from
307+ self, which would in that case be the normal bug model object, which
308+ would then hit the database to get the tasks.
309+ """
310+ # Copied from Bug.getBugTarget to avoid importing.
311+ for bugtask in self.bugtasks:
312+ if bugtask.target == target:
313+ return bugtask
314+ return None
315+
316+ @property
317+ def bugtask(self):
318+ """Return the bugtask for the branch project, or the default bugtask.
319+
320+ This method defers the identitication of the appropriate task to the
321+ branch target.
322+ """
323+ return self.branch.target.getBugTask(self)
324+
325+
326+class DecoratedBranch(BzrIdentityMixin):
327+ """Wrap a number of the branch accessors to cache results.
328+
329+ This avoids repeated db queries.
330+ """
331+ delegates(IBranch, 'branch')
332+
333+ def __init__(self, branch):
334+ self.branch = branch
335+
336+ @cachedproperty
337+ def linked_bugs(self):
338+ """Override the default behaviour of the branch object.
339+
340+ The default behaviour is just to get the bugs. We want to display the
341+ tasks however, and to avoid the extra database queries to get the
342+ tasks, we get them all at once, and provide decorated bugs (that have
343+ their tasks cached).
344+ """
345+ bugs = defaultdict(list)
346+ for bug, task in self.branch.getLinkedBugsAndTasks():
347+ bugs[bug].append(task)
348+ return [DecoratedBug(bug, self.branch, tasks)
349+ for bug, tasks in bugs.iteritems()
350+ if check_permission('launchpad.View', bug)]
351+
352+ @property
353+ def displayname(self):
354+ """Override the default model property.
355+
356+ If left to the underlying model, it would call the bzr_identity on the
357+ underlying branch rather than the cached bzr_identity on the decorated
358+ branch. And that would cause two database queries.
359+ """
360+ return self.bzr_identity
361+
362+ @cachedproperty
363+ def bzr_identity(self):
364+ """Cache the result of the bzr identity.
365+
366+ The property is defined in the bzrIdentityMixin class. This uses the
367+ associatedProductSeries and associatedSuiteSourcePackages methods.
368+ """
369+ return super(DecoratedBranch, self).bzr_identity
370+
371+ @cachedproperty
372+ def is_series_branch(self):
373+ """A simple property to see if there are any series links."""
374+ # True if linked to a product series or suite source package.
375+ return (
376+ len(self.associated_product_series) > 0 or
377+ len(self.suite_source_packages) > 0)
378+
379+ def associatedProductSeries(self):
380+ """Override the IBranch.associatedProductSeries."""
381+ return self.associated_product_series
382+
383+ def associatedSuiteSourcePackages(self):
384+ """Override the IBranch.associatedSuiteSourcePackages."""
385+ return self.suite_source_packages
386+
387+ @cachedproperty
388+ def associated_product_series(self):
389+ """Cache the realized product series links."""
390+ return list(self.branch.associatedProductSeries())
391+
392+ @cachedproperty
393+ def suite_source_packages(self):
394+ """Cache the realized suite source package links."""
395+ return list(self.branch.associatedSuiteSourcePackages())
396+
397+ @cachedproperty
398+ def upgrade_pending(self):
399+ """Cache the result as the property hits the database."""
400+ return self.branch.upgrade_pending
401+
402+ @cachedproperty
403+ def subscriptions(self):
404+ """Cache the realized branch subscription objects."""
405+ return list(self.branch.subscriptions)
406+
407+ def hasSubscription(self, user):
408+ """Override the default branch implementation.
409+
410+ The default implementation hits the database. Since we have a full
411+ list of subscribers anyway, a simple check over the list is
412+ sufficient.
413+ """
414+ for sub in self.subscriptions:
415+ if sub.person == user:
416+ return True
417+ return False
418+
419+ @cachedproperty
420+ def latest_revisions(self):
421+ """Cache the query result.
422+
423+ When a tal:repeat is used, the method is called twice. Firstly to
424+ check that there is something to iterate over, and secondly for the
425+ actual iteration. Without the cached property, the database is hit
426+ twice.
427+ """
428+ return list(self.branch.latest_revisions())