Launchpad itself

Merge lp:~deryck/launchpad/fewer-milestone-queries-bugtask-index into lp:launchpad

  1. fewer-milestone-queries-bugtask-index
  2. Merge into devel
Proposed by Deryck Hodge
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 11638
Proposed branch: lp:~deryck/launchpad/fewer-milestone-queries-bugtask-index
Merge into: lp:launchpad
Diff against target: 145 lines (+30/-22)
3 files modified
lib/canonical/widgets/lazrjs.py (+11/-6)
lib/lp/bugs/browser/bugtask.py (+12/-4)
lib/lp/bugs/browser/tests/test_bugtask.py (+7/-12)
To merge this branch: bzr merge lp:~deryck/launchpad/fewer-milestone-queries-bugtask-index
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Review via email: mp+36458@code.launchpad.net

Commit message

Reduce the query count for bugtask+index by 2 queries by being a bit smarter about how we get Milestone info.

Description of the change

Inspired by Robert's work related to fixing bug 636158 (timeouts on
bugtask+index), I've been trying to find spots to further reduce the
query count. This is a small branch that fixes up how we get
milestone data for the bug page and reduces the overall query count
for bugtask+index by 2 queries.

It's small, I know, but hey this can land on its own from other work
I'm doing. :-)

To do this, I fixed up the method that looks up the milestones for
the widget and passed in the milestones themselves rather than a
base vocabulary object. I ran a liberal test expression to make
sure this has no fallout:

./bin/test -cvvt "milestone|vocab"

I also lowered the query count in the test Robert added in
TestBugTaskView by 2 queries. To confirm, run:

./bin/test -cvvt TestBugTaskView

The test really only has the one query count change. The rest is me
fixing up lint.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Great, a few thoughts.
safe_hasattr is generally better than hasattr because it swallows less exceptions.

Doing a count() query might be ok if we were to then only read *some* milestones, but as the page shows today, my understanding is we'll always read all milestones - so the change to listify and use len() makes sense to me - but it might be worth a small comment saying that this is what goes on to aid future selves.

review: Approve
Revision history for this message
Deryck Hodge (deryck) wrote :

I've made both these changes. Thanks for the review, Robert!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/widgets/lazrjs.py'
2--- lib/canonical/widgets/lazrjs.py 2010-02-23 14:07:54 +0000
3+++ lib/canonical/widgets/lazrjs.py 2010-09-24 18:29:43 +0000
4@@ -21,6 +21,7 @@
5 from zope.schema.vocabulary import getVocabularyRegistry
6
7 from lazr.restful.interfaces import IWebServiceClientRequest
8+from canonical.lazr.utils import safe_hasattr
9
10 from canonical.launchpad.webapp.interfaces import ILaunchBag
11 from canonical.launchpad.webapp.publisher import canonical_url
12@@ -359,24 +360,28 @@
13 disabled_items = []
14 items = []
15 for item in vocab:
16+ # Introspect to make sure we're dealing with the object itself.
17+ # SimpleTerm objects have the object itself at item.value.
18+ if safe_hasattr(item, 'value'):
19+ item = item.value
20 if name_fn is not None:
21- name = name_fn(item.value)
22+ name = name_fn(item)
23 else:
24- name = item.value.title
25+ name = item.title
26 if value_fn is not None:
27- value = value_fn(item.value)
28+ value = value_fn(item)
29 else:
30- value = item.value.title
31+ value = item.title
32 new_item = {
33 'name': name,
34 'value': value,
35 'style': '', 'help': '', 'disabled': False}
36 for disabled_item in disabled_items:
37- if disabled_item == item.value:
38+ if disabled_item == item:
39 new_item['disabled'] = True
40 break
41 if css_class_prefix is not None:
42- new_item['css_class'] = css_class_prefix + item.value.name
43+ new_item['css_class'] = css_class_prefix + item.name
44 items.append(new_item)
45
46 if as_json:
47
48=== modified file 'lib/lp/bugs/browser/bugtask.py'
49--- lib/lp/bugs/browser/bugtask.py 2010-09-21 10:03:47 +0000
50+++ lib/lp/bugs/browser/bugtask.py 2010-09-24 18:29:43 +0000
51@@ -3608,12 +3608,17 @@
52
53 return items
54
55+ @cachedproperty
56+ def _visible_milestones(self):
57+ """The visible milestones for this context."""
58+ return MilestoneVocabulary(self.context).visible_milestones
59+
60 @property
61 def milestone_widget_items(self):
62 """The available milestone items as JSON."""
63 if self.user is not None:
64 items = vocabulary_to_choice_edit_items(
65- MilestoneVocabulary(self.context),
66+ self._visible_milestones,
67 value_fn=lambda item: canonical_url(
68 item, request=IWebServiceClientRequest(self.request)))
69 items.append({
70@@ -3627,9 +3632,12 @@
71
72 @cachedproperty
73 def target_has_milestones(self):
74- """Are there any milestones we can target?"""
75- return MilestoneVocabulary.getMilestoneTarget(
76- self.context).has_milestones
77+ """Are there any milestones we can target?
78+
79+ We always look up all milestones, so there's no harm
80+ using len on the list here and avoid the COUNT query.
81+ """
82+ return len(self._visible_milestones) > 0
83
84 def bugtask_canonical_url(self):
85 """Return the canonical url for the bugtask."""
86
87=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
88--- lib/lp/bugs/browser/tests/test_bugtask.py 2010-09-22 20:56:52 +0000
89+++ lib/lp/bugs/browser/tests/test_bugtask.py 2010-09-24 18:29:43 +0000
90@@ -6,16 +6,12 @@
91 __metaclass__ = type
92
93
94-from contextlib import nested
95 from doctest import DocTestSuite
96 import unittest
97
98 from storm.store import Store
99-from testtools.matchers import (
100- Equals,
101- LessThan,
102- MatchesAny,
103- )
104+from testtools.matchers import LessThan
105+
106 from zope.security.proxy import removeSecurityProxy
107
108 from canonical.launchpad.ftests import (
109@@ -33,7 +29,6 @@
110 from canonical.testing import LaunchpadFunctionalLayer
111 from lp.bugs.browser import bugtask
112 from lp.bugs.browser.bugtask import (
113- BugTaskView,
114 BugTaskEditView,
115 BugTasksAndNominationsView,
116 )
117@@ -62,23 +57,23 @@
118
119 def test_rendered_query_counts_constant_with_team_memberships(self):
120 login(ADMIN_EMAIL)
121- bugtask = self.factory.makeBugTask()
122+ task = self.factory.makeBugTask()
123 person_no_teams = self.factory.makePerson(password='test')
124 person_with_teams = self.factory.makePerson(password='test')
125 for _ in range(10):
126 self.factory.makeTeam(members=[person_with_teams])
127 # count with no teams
128- url = canonical_url(bugtask)
129+ url = canonical_url(task)
130 recorder = QueryCollector()
131 recorder.register()
132 self.addCleanup(recorder.unregister)
133- self.invalidate_caches(bugtask)
134+ self.invalidate_caches(task)
135 self.getUserBrowser(url, person_no_teams)
136 # This may seem large: it is; there is easily another 30% fat in there.
137- self.assertThat(recorder, HasQueryCount(LessThan(64)))
138+ self.assertThat(recorder, HasQueryCount(LessThan(62)))
139 count_with_no_teams = recorder.count
140 # count with many teams
141- self.invalidate_caches(bugtask)
142+ self.invalidate_caches(task)
143 self.getUserBrowser(url, person_with_teams)
144 # Allow an increase of one because storm bug 619017 causes additional
145 # queries, revalidating things unnecessarily. An increase which is