launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02524
[Merge] lp:~lifeless/launchpad/bug-661988 into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/bug-661988 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#661988 Timeout on Distribution:+bugs search
https://bugs.launchpad.net/bugs/661988
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-661988/+merge/48740
Reduce the time taken for distro scope bug searches by 50% by using sequence-of-sets based eager loading rather than wide-query eager loading. As part of making this fit cleanly stop loading assignees during bug task search (they are not rendered so not needed) - which removes an unneeded query grabbing data we don't use. Bug assignees are still catered for for visibility - the assignee eager loading was unnecessary and a pessimism.
--
https://code.launchpad.net/~lifeless/launchpad/bug-661988/+merge/48740
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-661988 into lp:launchpad.
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-01-31 16:58:27 +0000
+++ lib/lp/bugs/configure.zcml 2011-02-06 22:16:20 +0000
@@ -26,11 +26,6 @@
<lp:help-folder
folder="help" type="lp.bugs.publisher.BugsLayer" />
- <class class="lp.bugs.model.bugtask.BugTaskResultSet">
- <allow interface="storm.zope.interfaces.IResultSet" />
- <allow attributes="__getslice__" />
- </class>
-
<class
class="lp.bugs.model.bugactivity.BugActivity">
<allow
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-02-01 15:14:57 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-02-06 22:16:20 +0000
@@ -9,7 +9,6 @@
__all__ = [
'BugTaskDelta',
- 'BugTaskResultSet',
'BugTaskToBugAdapter',
'BugTaskMixin',
'BugTask',
@@ -426,21 +425,6 @@
self.bug.id, self.bugtargetdisplayname, self.bug.title)
-class BugTaskResultSet(DecoratedResultSet):
- """Decorated results with cached assignees."""
-
- def __iter__(self, *args, **kwargs):
- """Iter with caching of assignees.
-
- Assumes none of the decorators will need to access the assignees or
- there is not benefit.
- """
- bugtasks = list(
- super(BugTaskResultSet, self).__iter__(*args, **kwargs))
- BugTaskSet._cache_assignees(bugtasks)
- return iter(bugtasks)
-
-
def BugTaskToBugAdapter(bugtask):
"""Adapt an IBugTask to an IBug."""
return bugtask.bug
@@ -2371,16 +2355,17 @@
origin.append(table)
return origin
- def _search(self, resultrow, prejoins, params, *args):
+ def _search(self, resultrow, prejoins, pre_iter_hook, params, *args):
"""Return a Storm result set for the given search parameters.
:param resultrow: The type of data returned by the query.
:param prejoins: A sequence of Storm SQL row instances which are
pre-joined.
+ :param pre_iter_hook: An optional pre-iteration hook used for eager
+ loading bug targets for list views.
:param params: A BugTaskSearchParams instance.
:param args: optional additional BugTaskSearchParams instances,
"""
-
store = IStore(BugTask)
[query, clauseTables, orderby, bugtask_decorator, join_tables,
has_duplicate_results] = self.buildQuery(params)
@@ -2400,7 +2385,8 @@
decorator = bugtask_decorator
resultset.order_by(orderby)
- return DecoratedResultSet(resultset, result_decorator=decorator)
+ return DecoratedResultSet(resultset, result_decorator=decorator,
+ pre_iter_hook=pre_iter_hook)
bugtask_fti = SQL('BugTask.fti')
inner_resultrow = (BugTask, bugtask_fti)
@@ -2439,8 +2425,8 @@
result = store.using(*origin).find(resultrow)
result.order_by(orderby)
- return BugTaskResultSet(
- result, result_decorator=decorator)
+ return DecoratedResultSet(result, result_decorator=decorator,
+ pre_iter_hook=pre_iter_hook)
def search(self, params, *args, **kwargs):
"""See `IBugTaskSet`.
@@ -2459,35 +2445,37 @@
if _noprejoins:
prejoins = []
resultrow = BugTask
+ eager_load = None
else:
requested_joins = kwargs.get('prejoins', [])
+ # NB: We could save later work by predicting what sort of targets
+ # we might be interested in here, but as at any point we're dealing
+ # with relatively few results, this is likely to be a small win.
prejoins = [
- (Bug, LeftJoin(Bug, BugTask.bug == Bug.id)),
- (Product, LeftJoin(Product, BugTask.product == Product.id)),
- (SourcePackageName,
- LeftJoin(
- SourcePackageName,
- BugTask.sourcepackagename == SourcePackageName.id)),
+ (Bug, LeftJoin(Bug, BugTask.bug == Bug.id))
] + requested_joins
- resultrow = (BugTask, Bug, Product, SourcePackageName)
+ def eager_load(results):
+ product_ids = set([row[0].productID for row in results])
+ product_ids.discard(None)
+ pkgname_ids = set(
+ [row[0].sourcepackagenameID for row in results])
+ pkgname_ids.discard(None)
+ store = IStore(BugTask)
+ if product_ids:
+ list(store.find(Product, Product.id.is_in(product_ids)))
+ if pkgname_ids:
+ list(store.find(SourcePackageName,
+ SourcePackageName.id.is_in(pkgname_ids)))
+ resultrow = (BugTask, Bug)
additional_result_objects = [
table for table, join in requested_joins
if table not in resultrow]
resultrow = resultrow + tuple(additional_result_objects)
- return self._search(resultrow, prejoins, params, *args)
+ return self._search(resultrow, prejoins, eager_load, params, *args)
def searchBugIds(self, params):
"""See `IBugTaskSet`."""
- return self._search(BugTask.bugID, [], params).result_set
-
- @staticmethod
- def _cache_assignees(rows):
- assignee_ids = set(
- bug_task.assigneeID for bug_task in rows)
- assignees = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
- assignee_ids, need_validity=True)
- # Execute query to load storm cache.
- list(assignees)
+ return self._search(BugTask.bugID, [], None, params).result_set
def getPrecachedNonConjoinedBugTasks(self, user, milestone):
"""See `IBugTaskSet`."""
@@ -2495,10 +2483,7 @@
user, milestone=milestone,
orderby=['status', '-importance', 'id'],
omit_dupes=True, exclude_conjoined_tasks=True)
- non_conjoined_slaves = self.search(params)
-
- return DecoratedResultSet(
- non_conjoined_slaves, pre_iter_hook=BugTaskSet._cache_assignees)
+ return self.search(params)
def createTask(self, bug, owner, product=None, productseries=None,
distribution=None, distroseries=None,
=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py 2011-02-01 15:14:57 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py 2011-02-06 22:16:20 +0000
@@ -34,10 +34,7 @@
BugTaskStatus,
IBugTaskSet,
)
-from lp.bugs.model.bugtask import (
- BugTask,
- BugTaskResultSet,
- )
+from lp.bugs.model.bugtask import BugTask
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.distributionsourcepackage import (
IDistributionSourcePackage,
@@ -968,8 +965,11 @@
found_tasks = self.runSearch(
params,
prejoins=[(Person, Join(Person, BugTask.owner == Person.id))])
+ # More than one query may have been performed
+ search_count = recorder.count
+ # Accessing the owner does not trigger more queries.
found_tasks[0].owner
- self.assertThat(recorder, HasQueryCount(Equals(1)))
+ self.assertThat(recorder, HasQueryCount(Equals(search_count)))
class NoPreloadBugtaskTargets(MultipleParams):
@@ -994,65 +994,6 @@
return [bugtask.bug.id for bugtask in expected_bugtasks]
-class TestCachingAssignees(TestCaseWithFactory):
- """Searching bug tasks should pre-cache the bugtask assignees."""
-
- layer = LaunchpadFunctionalLayer
-
- def setUp(self):
- super(TestCachingAssignees, self).setUp()
- self.owner = self.factory.makePerson(name="bug-owner")
- with person_logged_in(self.owner):
- # Create some bugs with assigned bugtasks.
- self.bug = self.factory.makeBug(
- owner=self.owner)
- self.bug.default_bugtask.transitionToAssignee(
- self.factory.makePerson())
-
- for i in xrange(9):
- bugtask = self.factory.makeBugTask(
- bug=self.bug)
- bugtask.transitionToAssignee(
- self.factory.makePerson())
- self.bug.setPrivate(True, self.owner)
-
- def _get_bug_tasks(self):
- """Get the bugtasks for a bug.
-
- This method is used rather than Bug.bugtasks since the later does
- prejoining which would spoil the test.
- """
- store = Store.of(self.bug)
- return store.find(
- BugTask, BugTask.bug == self.bug)
-
- def test_no_precaching(self):
- bugtasks = self._get_bug_tasks()
- Store.of(self.bug).invalidate()
- with person_logged_in(self.owner):
- with StormStatementRecorder() as recorder:
- # Access the assignees to trigger a query.
- names = [bugtask.assignee.name for bugtask in bugtasks]
- # With no caching, the number of queries is roughly twice the
- # number of bugtasks.
- query_count_floor = len(names) * 2
- self.assertThat(
- recorder, HasQueryCount(Not(LessThan(query_count_floor))))
-
- def test_precaching(self):
- bugtasks = self._get_bug_tasks()
- Store.of(self.bug).invalidate()
- with person_logged_in(self.owner):
- with StormStatementRecorder() as recorder:
- bugtasks = BugTaskResultSet(bugtasks)
- # Access the assignees to trigger a query if not properly
- # cached.
- [bugtask.assignee.name for bugtask in bugtasks]
- # With caching the number of queries is two, one for the
- # bugtask and one for all of the assignees at once.
- self.assertThat(recorder, HasQueryCount(Equals(2)))
-
-
def test_suite():
suite = unittest.TestSuite()
loader = unittest.TestLoader()
