Launchpad itself

Merge lp://staging/~thumper/launchpad/bmp-index-faster into lp://staging/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://staging/~thumper/launchpad/bmp-index-faster
Merge into: lp://staging/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://staging/~thumper/launchpad/bmp-index-faster
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Review via email: mp+27788@code.staging.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
The diff is not available at this time. You can reload the page or download it.