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
=== modified file 'lib/canonical/widgets/lazrjs.py'
--- lib/canonical/widgets/lazrjs.py 2010-02-23 14:07:54 +0000
+++ lib/canonical/widgets/lazrjs.py 2010-09-24 18:29:43 +0000
@@ -21,6 +21,7 @@
21from zope.schema.vocabulary import getVocabularyRegistry21from zope.schema.vocabulary import getVocabularyRegistry
2222
23from lazr.restful.interfaces import IWebServiceClientRequest23from lazr.restful.interfaces import IWebServiceClientRequest
24from canonical.lazr.utils import safe_hasattr
2425
25from canonical.launchpad.webapp.interfaces import ILaunchBag26from canonical.launchpad.webapp.interfaces import ILaunchBag
26from canonical.launchpad.webapp.publisher import canonical_url27from canonical.launchpad.webapp.publisher import canonical_url
@@ -359,24 +360,28 @@
359 disabled_items = []360 disabled_items = []
360 items = []361 items = []
361 for item in vocab:362 for item in vocab:
363 # Introspect to make sure we're dealing with the object itself.
364 # SimpleTerm objects have the object itself at item.value.
365 if safe_hasattr(item, 'value'):
366 item = item.value
362 if name_fn is not None:367 if name_fn is not None:
363 name = name_fn(item.value)368 name = name_fn(item)
364 else:369 else:
365 name = item.value.title370 name = item.title
366 if value_fn is not None:371 if value_fn is not None:
367 value = value_fn(item.value)372 value = value_fn(item)
368 else:373 else:
369 value = item.value.title374 value = item.title
370 new_item = {375 new_item = {
371 'name': name,376 'name': name,
372 'value': value,377 'value': value,
373 'style': '', 'help': '', 'disabled': False}378 'style': '', 'help': '', 'disabled': False}
374 for disabled_item in disabled_items:379 for disabled_item in disabled_items:
375 if disabled_item == item.value:380 if disabled_item == item:
376 new_item['disabled'] = True381 new_item['disabled'] = True
377 break382 break
378 if css_class_prefix is not None:383 if css_class_prefix is not None:
379 new_item['css_class'] = css_class_prefix + item.value.name384 new_item['css_class'] = css_class_prefix + item.name
380 items.append(new_item)385 items.append(new_item)
381386
382 if as_json:387 if as_json:
383388
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2010-09-21 10:03:47 +0000
+++ lib/lp/bugs/browser/bugtask.py 2010-09-24 18:29:43 +0000
@@ -3608,12 +3608,17 @@
36083608
3609 return items3609 return items
36103610
3611 @cachedproperty
3612 def _visible_milestones(self):
3613 """The visible milestones for this context."""
3614 return MilestoneVocabulary(self.context).visible_milestones
3615
3611 @property3616 @property
3612 def milestone_widget_items(self):3617 def milestone_widget_items(self):
3613 """The available milestone items as JSON."""3618 """The available milestone items as JSON."""
3614 if self.user is not None:3619 if self.user is not None:
3615 items = vocabulary_to_choice_edit_items(3620 items = vocabulary_to_choice_edit_items(
3616 MilestoneVocabulary(self.context),3621 self._visible_milestones,
3617 value_fn=lambda item: canonical_url(3622 value_fn=lambda item: canonical_url(
3618 item, request=IWebServiceClientRequest(self.request)))3623 item, request=IWebServiceClientRequest(self.request)))
3619 items.append({3624 items.append({
@@ -3627,9 +3632,12 @@
36273632
3628 @cachedproperty3633 @cachedproperty
3629 def target_has_milestones(self):3634 def target_has_milestones(self):
3630 """Are there any milestones we can target?"""3635 """Are there any milestones we can target?
3631 return MilestoneVocabulary.getMilestoneTarget(3636
3632 self.context).has_milestones3637 We always look up all milestones, so there's no harm
3638 using len on the list here and avoid the COUNT query.
3639 """
3640 return len(self._visible_milestones) > 0
36333641
3634 def bugtask_canonical_url(self):3642 def bugtask_canonical_url(self):
3635 """Return the canonical url for the bugtask."""3643 """Return the canonical url for the bugtask."""
36363644
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2010-09-22 20:56:52 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2010-09-24 18:29:43 +0000
@@ -6,16 +6,12 @@
6__metaclass__ = type6__metaclass__ = type
77
88
9from contextlib import nested
10from doctest import DocTestSuite9from doctest import DocTestSuite
11import unittest10import unittest
1211
13from storm.store import Store12from storm.store import Store
14from testtools.matchers import (13from testtools.matchers import LessThan
15 Equals,14
16 LessThan,
17 MatchesAny,
18 )
19from zope.security.proxy import removeSecurityProxy15from zope.security.proxy import removeSecurityProxy
2016
21from canonical.launchpad.ftests import (17from canonical.launchpad.ftests import (
@@ -33,7 +29,6 @@
33from canonical.testing import LaunchpadFunctionalLayer29from canonical.testing import LaunchpadFunctionalLayer
34from lp.bugs.browser import bugtask30from lp.bugs.browser import bugtask
35from lp.bugs.browser.bugtask import (31from lp.bugs.browser.bugtask import (
36 BugTaskView,
37 BugTaskEditView,32 BugTaskEditView,
38 BugTasksAndNominationsView,33 BugTasksAndNominationsView,
39 )34 )
@@ -62,23 +57,23 @@
6257
63 def test_rendered_query_counts_constant_with_team_memberships(self):58 def test_rendered_query_counts_constant_with_team_memberships(self):
64 login(ADMIN_EMAIL)59 login(ADMIN_EMAIL)
65 bugtask = self.factory.makeBugTask()60 task = self.factory.makeBugTask()
66 person_no_teams = self.factory.makePerson(password='test')61 person_no_teams = self.factory.makePerson(password='test')
67 person_with_teams = self.factory.makePerson(password='test')62 person_with_teams = self.factory.makePerson(password='test')
68 for _ in range(10):63 for _ in range(10):
69 self.factory.makeTeam(members=[person_with_teams])64 self.factory.makeTeam(members=[person_with_teams])
70 # count with no teams65 # count with no teams
71 url = canonical_url(bugtask)66 url = canonical_url(task)
72 recorder = QueryCollector()67 recorder = QueryCollector()
73 recorder.register()68 recorder.register()
74 self.addCleanup(recorder.unregister)69 self.addCleanup(recorder.unregister)
75 self.invalidate_caches(bugtask)70 self.invalidate_caches(task)
76 self.getUserBrowser(url, person_no_teams)71 self.getUserBrowser(url, person_no_teams)
77 # This may seem large: it is; there is easily another 30% fat in there.72 # This may seem large: it is; there is easily another 30% fat in there.
78 self.assertThat(recorder, HasQueryCount(LessThan(64)))73 self.assertThat(recorder, HasQueryCount(LessThan(62)))
79 count_with_no_teams = recorder.count74 count_with_no_teams = recorder.count
80 # count with many teams75 # count with many teams
81 self.invalidate_caches(bugtask)76 self.invalidate_caches(task)
82 self.getUserBrowser(url, person_with_teams)77 self.getUserBrowser(url, person_with_teams)
83 # Allow an increase of one because storm bug 619017 causes additional78 # Allow an increase of one because storm bug 619017 causes additional
84 # queries, revalidating things unnecessarily. An increase which is79 # queries, revalidating things unnecessarily. An increase which is