Merge lp:~edwin-grubbs/launchpad/bug-638924-milestone-page-eager-loading into lp:launchpad

[Edwin Grubbs]

[r=adeuring][ui=none][bug=638924] Fixed timeout on ubuntu milestone +index page.

Summary
-------

This branch fixes timeouts viewing a distro's milestone +index page with
a ton of bugs. The number of queries will be constant for a milestone
page on a distro or a project. The milestone page for a project group
will make two additional queries for each of its project that has a
milestone with a matching name.

Implementation details
----------------------

Added search functionality to BugTaskSet.search() to eliminate the need
to call getConjoinedMaster() on each bug task found. Also, precached all
the bug assignees and their validity. (Validity is used by tales to
decide whether the person icon should be grayed out.)
    lib/lp/bugs/interfaces/bugtask.py
    lib/lp/bugs/model/bugtask.py
    lib/lp/registry/browser/milestone.py
    lib/lp/registry/browser/tests/test_milestone.py

Moved code for precaching various person attributes from the Person
class into the PersonSet class so that it can be more easily reused.
    lib/lp/registry/interfaces/person.py
    lib/lp/registry/model/person.py

Tests
-----

./bin/test -vv -t 'test_milestone|milestone-views.txt'

Demo and Q/A
------------

* Open https://qastaging.launchpad.net/ubuntu/+milestone/ubuntu-9.10/
    * It should not timeout, and the query count should be under 40.
* Open https://qastaging.launchpad.net/launchpad-project/+milestone/10.12
    * It should not timeout, and the query count should be under 80.
* Open https://qastaging.launchpad.net/launchpad-registry/+milestone/10.12
    * It should not timeout, and the query count should be under 40.

Lint
----

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/interfaces/bugtask.py
  lib/lp/bugs/model/bugtask.py
  lib/lp/registry/browser/milestone.py
  lib/lp/registry/browser/tests/test_milestone.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/model/person.py

./lib/lp/registry/interfaces/person.py
     493: E302 expected 2 blank lines, found 1
    2095: E302 expected 2 blank lines, found 3

[Abel Deuring (adeuring)]

Hi Edwin,

this is a very nice branch! I just have a few remarks, nothing serious.

> + def getPrecachedNonConjoinedBugTasks(self, user, milestone):
> + """See `IBugTaskSet`."""
> + params = BugTaskSearchParams(
> + user, milestone=milestone,
> + orderby=['status', '-importance', 'id'],
> + omit_dupes=True, exclude_conjoined_tasks=True)
> + non_conjoined_slaves = self.search(params)
> +
> + def cache_people(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 DecoratedResultSet(
> + non_conjoined_slaves, pre_iter_hook=cache_people)
> +

Preloading the Person records this way is fine, but did you consider
to use the optional BugTaskSet.search() parameter prejoins? Like so:

    self.search(
        params, prejoins=[(Person, LeftJoin(BugTask.assignee==Person.id)])

Seeing that you implemented another prejoin mechanism in PersonSet,
I get the impression that we should perhaps "normalize" the approaches
to preload records. But that's something for future branches.

> +class TestProjectGroupMilestoneIndexQueryCount(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer

The tests for the different milestone targets a quite similar. Did
you consider to use a comon base class containing the test methods
and to setup the milestone, milestone target etc in derived classes?

review: Approve (code)

[Edwin Grubbs (edwin-grubbs)]

> Hi Edwin,
>
> this is a very nice branch! I just have a few remarks, nothing serious.
>
>
>
> > + def getPrecachedNonConjoinedBugTasks(self, user, milestone):
> > + """See `IBugTaskSet`."""
> > + params = BugTaskSearchParams(
> > + user, milestone=milestone,
> > + orderby=['status', '-importance', 'id'],
> > + omit_dupes=True, exclude_conjoined_tasks=True)
> > + non_conjoined_slaves = self.search(params)
> > +
> > + def cache_people(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 DecoratedResultSet(
> > + non_conjoined_slaves, pre_iter_hook=cache_people)
> > +
>
> Preloading the Person records this way is fine, but did you consider
> to use the optional BugTaskSet.search() parameter prejoins? Like so:
>
> self.search(
> params, prejoins=[(Person, LeftJoin(BugTask.assignee==Person.id)])

I thought about it, but I think there are some advantages to not loading a bunch of duplicate data since many of the bugs will share the same assignee. I also needed the Person.is_valid_person attribute to be precached, and this can't be done with a simple prejoin since that attribute is not handled by storm.

> Seeing that you implemented another prejoin mechanism in PersonSet,
> I get the impression that we should perhaps "normalize" the approaches
> to preload records. But that's something for future branches.

I didn't actually implement that. The code was already there for precaching info in Team.allmembers. I just moved some of the code to PersonSet where it could be reused more easily.

> > +class TestProjectGroupMilestoneIndexQueryCount(TestCaseWithFactory):
> > +
> > + layer = DatabaseFunctionalLayer
>
> The tests for the different milestone targets a quite similar. Did
> you consider to use a comon base class containing the test methods
> and to setup the milestone, milestone target etc in derived classes?

I have refactored some of the redundancy out, but I left the test methods in each class, since the number of queries may differ for each milestone target type.

I also added a test for PersonSet.getPrecachedPersonsFromIDs().

[Edwin Grubbs (edwin-grubbs)]

Here is the incremental diff:

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py 2010-12-03 03:03:14 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py 2010-12-03 16:27:46 +0000
@@ -169,7 +169,39 @@
         self.assertEqual(0, product.development_focus.all_bugtasks.count())

-class TestProjectMilestoneIndexQueryCount(TestCaseWithFactory):
+class TestQueryCountBase(TestCaseWithFactory):
+
+ def assert_bugtasks_query_count(self, milestone, bugtask_count,
+ query_limit):
+ # Assert that the number of queries run by view.bugtasks is low.
+ self.add_bug(bugtask_count)
+ login_person(self.owner)
+ view = create_initialized_view(milestone, '+index')
+ # Eliminate permission check for the admin team from the
+ # recorded queries by loading it now. If the test ever breaks,
+ # the person fixing it won't waste time trying to track this
+ # query down.
+ getUtility(ILaunchpadCelebrities).admin
+ with StormStatementRecorder() as recorder:
+ bugtasks = list(view.bugtasks)
+ self.assertThat(recorder, HasQueryCount(LessThan(query_limit)))
+ self.assertEqual(bugtask_count, len(bugtasks))
+
+ def assert_milestone_page_query_count(self, milestone, query_limit):
+ collector = QueryCollector()
+ collector.register()
+ try:
+ milestone_url = canonical_url(milestone)
+ browser = self.getUserBrowser(user=self.owner)
+ browser.open(milestone_url)
+ self.assertThat(collector, HasQueryCount(LessThan(query_limit)))
+ finally:
+ # Unregister now in case this method is called multiple
+ # times in a single test.
+ collector.unregister()
+
+
+class TestProjectMilestoneIndexQueryCount(TestQueryCountBase):

     layer = DatabaseFunctionalLayer

@@ -182,8 +214,8 @@
             storm_cache_size: 1000
             '''))
         self.addCleanup(config.pop, 'storm-cache')
- owner = self.factory.makePerson(name='product-owner')
- self.product = self.factory.makeProduct(owner=owner)
+ self.owner = self.factory.makePerson(name='product-owner')
+ self.product = self.factory.makeProduct(owner=self.owner)
         login_person(self.product.owner)
         self.milestone = self.factory.makeMilestone(
             productseries=self.product.development_focus)
@@ -206,36 +238,19 @@
         # 3. Load links to specifications.
         # 4. Load links to branches.
         bugtask_count = 10
- self.add_bug(bugtask_count)
- login_person(self.product.owner)
- view = create_initialized_view(self.milestone, '+index')
- # Eliminate permission check for the admin team from the
- # recorded queries by loading it now. If the test ever breaks,
- # the person fixing it won't waste time trying to track this
- # query down.
- getUtility(ILaunchpadCelebrities).admin
- with StormStatementRecorder() as recorder:
- bugtasks = list(view.bugtasks)
- self.assertThat(recorder, Has...

[Abel Deuring (adeuring)]

Hi Edwin,

thanks for the additional changes, r=me again. Just one nitpick:

> @@ -339,56 +354,29 @@
>
> def test_bugtasks_queries(self):
> # The view.bugtasks attribute will make four queries:

s/four/five/

review: Approve (code)

[Robert Collins (lifeless)]

I have one small comment... you've moved from Store.of(object) to
IStore - this is a problem because it will miss the storm cache if
some page/request determines it needs IMaster explicitly.

You need to pass the store into the precache help you've put on PersonSet.

-Rob

[Edwin Grubbs (edwin-grubbs)]

I have added a "store" parameter to the _getPrecachedPersons() so that Person._all_members() can specify it.

[Robert Collins (lifeless)]

Thanks!

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2010-12-01 11:26:57 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2010-12-09 15:28:31 +0000
@@ -1145,7 +1145,8 @@
                  hardware_owner_is_subscribed_to_bug=False,
                  hardware_is_linked_to_bug=False,
                  linked_branches=None, structural_subscriber=None,
-                 modified_since=None, created_since=None):
+                 modified_since=None, created_since=None,
+                 exclude_conjoined_tasks=False):
 
         self.bug = bug
         self.searchtext = searchtext
@@ -1191,6 +1192,7 @@
         self.structural_subscriber = structural_subscriber
         self.modified_since = modified_since
         self.created_since = created_since
+        self.exclude_conjoined_tasks = exclude_conjoined_tasks
 
     def setProduct(self, product):
         """Set the upstream context on which to filter the search."""
@@ -1530,6 +1532,12 @@
         This takes into account bug subscription filters.
         """
 
+    def getPrecachedNonConjoinedBugTasks(user, milestone):
+        """List of non-conjoined bugtasks targeted to the milestone.
+
+        The assignee and the assignee's validity are precached.
+        """
+
 
 def valid_remote_bug_url(value):
     """Verify that the URL is to a bug to a known bug tracker."""

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-12-01 11:26:57 +0000
+++ lib/lp/bugs/model/bugtask.py	2010-12-09 15:28:31 +0000
@@ -43,14 +43,11 @@
     Select,
     SQL,
     )
+from storm.info import ClassAlias
 from storm.store import (
     EmptyResultSet,
     Store,
     )
-from storm.zope.interfaces import (
-    IResultSet,
-    ISQLObjectResultSet,
-    )
 from zope.component import getUtility
 from zope.interface import (
     alsoProvides,
@@ -142,6 +139,7 @@
 from lp.registry.interfaces.milestone import IProjectGroupMilestone
 from lp.registry.interfaces.person import (
     IPerson,
+    IPersonSet,
     validate_person,
     validate_public_person,
     )
@@ -1490,16 +1488,23 @@
         from lp.bugs.model.bug import Bug
         from lp.bugs.model.bugbranch import BugBranch
 
-        bug_ids = list(set(bugtask.bugID for bugtask in bugtasks))
+        bug_ids = set(bugtask.bugID for bugtask in bugtasks)
         bug_ids_with_specifications = set(IStore(SpecificationBug).find(
             SpecificationBug.bugID,
             SpecificationBug.bugID.is_in(bug_ids)))
         bug_ids_with_branches = set(IStore(BugBranch).find(
                 BugBranch.bugID, BugBranch.bugID.is_in(bug_ids)))
 
-        # Cache all bugs at once to avoid one query per bugtask. We
-        # could rely on the Storm cache, but this is explicit.
-        bugs = dict(IStore(Bug).find((Bug.id, Bug), Bug.id.is_in(bug_ids)))
+        # Check if the bugs are cached. If not, cache all uncached bugs
+        # at once to avoid one query per bugtask. We could rely on the
+        # Storm cache, but this is explicit.
+        bugs = dict(
+            (bug.id, bug)
+            for bug in IStore(Bug).find(Bug, Bug.id.is_in(bug_ids)).cached())
+        uncached_ids = bug_ids.difference(bug_id for bug_id in bugs)
+        if len(uncached_ids) > 0:
+            bugs.update(dict(IStore(Bug).find((Bug.id, Bug),
+                                              Bug.id.is_in(uncached_ids))))
 
         badge_properties = {}
         for bugtask in bugtasks:
@@ -1665,6 +1670,48 @@
                 where_cond = search_value_to_where_condition(params.milestone)
             extra_clauses.append("BugTask.milestone %s" % where_cond)
 
+            if params.exclude_conjoined_tasks:
+                # Perform a LEFT JOIN to the conjoined master bugtask.
+                # If the conjoined master is not null, it gets filtered
+                # out.
+                ConjoinedMaster = ClassAlias(BugTask, 'ConjoinedMaster')
+                extra_clauses.append("ConjoinedMaster.id IS NULL")
+                if params.milestone.distribution is not None:
+                    current_series = (
+                        params.milestone.distribution.currentseries)
+                    join = LeftJoin(
+                        ConjoinedMaster,
+                        And(ConjoinedMaster.bug == BugTask.bugID,
+                            ConjoinedMaster.distroseries == current_series.id,
+                            ))
+                    join_tables.append((ConjoinedMaster, join))
+                else:
+                    # Prevent import loop.
+                    from lp.registry.model.product import Product
+                    if IProjectGroupMilestone.providedBy(params.milestone):
+                        dev_focus_ids = list(
+                            IStore(Product).find(
+                                Product.development_focusID,
+                                Product.project == params.milestone.target))
+                    elif params.milestone.product is not None:
+                        dev_focus_ids = [
+                            params.milestone.product.development_focusID]
+                    else:
+                        raise AssertionError(
+                            "A milestone must always have either a project, "
+                            "project group, or distribution")
+                    join = LeftJoin(
+                        ConjoinedMaster,
+                        And(ConjoinedMaster.bug == BugTask.bugID,
+                            ConjoinedMaster.productseriesID.is_in(
+                                dev_focus_ids)))
+                    join_tables.append((ConjoinedMaster, join))
+        elif params.exclude_conjoined_tasks:
+            raise ValueError(
+                "BugTaskSearchParam.exclude_conjoined cannot be True if "
+                "BugTaskSearchParam.milestone is not set")
+
+
         if params.project:
             # Prevent circular import problems.
             from lp.registry.model.product import Product
@@ -2309,6 +2356,25 @@
         """See `IBugTaskSet`."""
         return self._search(BugTask.bugID, [], params).result_set
 
+    def getPrecachedNonConjoinedBugTasks(self, user, milestone):
+        """See `IBugTaskSet`."""
+        params = BugTaskSearchParams(
+            user, milestone=milestone,
+            orderby=['status', '-importance', 'id'],
+            omit_dupes=True, exclude_conjoined_tasks=True)
+        non_conjoined_slaves = self.search(params)
+
+        def cache_people(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 DecoratedResultSet(
+            non_conjoined_slaves, pre_iter_hook=cache_people)
+
     def createTask(self, bug, owner, product=None, productseries=None,
                    distribution=None, distroseries=None,
                    sourcepackagename=None,

=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py	2010-11-23 23:22:27 +0000
+++ lib/lp/registry/browser/milestone.py	2010-12-09 15:28:31 +0000
@@ -41,7 +41,6 @@
     precache_permission_for_objects,
     )
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
-from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.menu import (
     ApplicationMenu,
     ContextMenu,
@@ -57,7 +56,6 @@
     )
 from lp.bugs.browser.bugtask import BugTaskListingItem
 from lp.bugs.interfaces.bugtask import (
-    BugTaskSearchParams,
     IBugTaskSet,
     )
 from lp.registry.browser import (
@@ -249,23 +247,11 @@
     @cachedproperty
     def _bugtasks(self):
         """The list of non-conjoined bugtasks targeted to this milestone."""
-        user = getUtility(ILaunchBag).user
-        params = BugTaskSearchParams(user, milestone=self.context,
-                    orderby=['status', '-importance', 'id'],
-                    omit_dupes=True)
-        tasks = getUtility(IBugTaskSet).search(params)
-        # We could replace all the code below with a simple
-        # >>> [task for task in tasks if task.conjoined_master is None]
-        # But that'd cause one extra hit to the DB for every bugtask returned
-        # by the search above, so we do a single query to get all of a task's
-        # siblings here and use that to find whether or not a given bugtask
-        # has a conjoined master.
-        bugs_and_tasks = getUtility(IBugTaskSet).getBugTasks(
-            [task.bug.id for task in tasks])
-        non_conjoined_slaves = []
-        for task in tasks:
-            if task.getConjoinedMaster(bugs_and_tasks[task.bug]) is None:
-                non_conjoined_slaves.append(task)
+        # Put the results in a list so that iterating over it multiple
+        # times in this method does not make multiple queries.
+        non_conjoined_slaves = list(
+            getUtility(IBugTaskSet).getPrecachedNonConjoinedBugTasks(
+                self.user, self.context))
         # Checking bug permissions is expensive. We know from the query that
         # the user has at least launchpad.View on the bugtasks and their bugs.
         precache_permission_for_objects(

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2010-10-26 15:47:24 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2010-12-09 15:28:31 +0000
@@ -5,9 +5,13 @@
 
 __metaclass__ = type
 
+from textwrap import dedent
+
 from testtools.matchers import LessThan
 from zope.component import getUtility
 
+from canonical.config import config
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp import canonical_url
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.bugs.interfaces.bugtask import IBugTaskSet
@@ -15,14 +19,16 @@
     ANONYMOUS,
     login,
     login_person,
+    login_team,
     logout,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
+from lp.testing._webservice import QueryCollector
 from lp.testing.matchers import HasQueryCount
 from lp.testing.memcache import MemcacheTestCase
 from lp.testing.views import create_initialized_view
-from lp.testing._webservice import QueryCollector
 
 
 class TestMilestoneViews(TestCaseWithFactory):
@@ -163,10 +169,89 @@
         self.assertEqual(0, product.development_focus.all_bugtasks.count())
 
 
-class TestMilestoneIndex(TestCaseWithFactory):
+class TestQueryCountBase(TestCaseWithFactory):
+
+    def assert_bugtasks_query_count(self, milestone, bugtask_count,
+                                    query_limit):
+        # Assert that the number of queries run by view.bugtasks is low.
+        self.add_bug(bugtask_count)
+        login_person(self.owner)
+        view = create_initialized_view(milestone, '+index')
+        # Eliminate permission check for the admin team from the
+        # recorded queries by loading it now. If the test ever breaks,
+        # the person fixing it won't waste time trying to track this
+        # query down.
+        getUtility(ILaunchpadCelebrities).admin
+        with StormStatementRecorder() as recorder:
+            bugtasks = list(view.bugtasks)
+        self.assertThat(recorder, HasQueryCount(LessThan(query_limit)))
+        self.assertEqual(bugtask_count, len(bugtasks))
+
+    def assert_milestone_page_query_count(self, milestone, query_limit):
+        collector = QueryCollector()
+        collector.register()
+        try:
+            milestone_url = canonical_url(milestone)
+            browser = self.getUserBrowser(user=self.owner)
+            browser.open(milestone_url)
+            self.assertThat(collector, HasQueryCount(LessThan(query_limit)))
+        finally:
+            # Unregister now in case this method is called multiple
+            # times in a single test.
+            collector.unregister()
+
+
+class TestProjectMilestoneIndexQueryCount(TestQueryCountBase):
 
     layer = DatabaseFunctionalLayer
 
+    def setUp(self):
+        super(TestProjectMilestoneIndexQueryCount, self).setUp()
+        # Increase cache size so that the query counts aren't affected
+        # by objects being removed from the cache early.
+        config.push('storm-cache', dedent('''
+            [launchpad]
+            storm_cache_size: 1000
+            '''))
+        self.addCleanup(config.pop, 'storm-cache')
+        self.owner = self.factory.makePerson(name='product-owner')
+        self.product = self.factory.makeProduct(owner=self.owner)
+        login_person(self.product.owner)
+        self.milestone = self.factory.makeMilestone(
+            productseries=self.product.development_focus)
+
+    def add_bug(self, count):
+        login_person(self.product.owner)
+        for i in range(count):
+            bug = self.factory.makeBug(product=self.product)
+            bug.bugtasks[0].transitionToMilestone(
+                self.milestone, self.product.owner)
+            # This is necessary to test precaching of assignees.
+            bug.bugtasks[0].transitionToAssignee(
+                self.factory.makePerson())
+        logout()
+
+    def test_bugtasks_queries(self):
+        # The view.bugtasks attribute will make four queries:
+        #  1. Load bugtasks and bugs.
+        #  2. Load assignees (Person, Account, and EmailAddress).
+        #  3. Load links to specifications.
+        #  4. Load links to branches.
+        bugtask_count = 10
+        self.assert_bugtasks_query_count(
+            self.milestone, bugtask_count, query_limit=5)
+
+    def test_milestone_eager_loading(self):
+        # Verify that the number of queries does not increase with more
+        # bugs with different assignees.
+        query_limit = 34
+        self.add_bug(3)
+        self.assert_milestone_page_query_count(
+            self.milestone, query_limit=query_limit)
+        self.add_bug(10)
+        self.assert_milestone_page_query_count(
+            self.milestone, query_limit=query_limit)
+
     def test_more_private_bugs_query_count_is_constant(self):
         # This test tests that as we add more private bugs to a milestone
         # index page, the number of queries issued by the page does not
@@ -175,6 +260,7 @@
         # is very large already, if the test fails due to such a change,
         # please cut some more of the existing fat out of it rather than
         # increasing the cap.
+        page_query_limit = 34
         product = self.factory.makeProduct()
         login_person(product.owner)
         milestone = self.factory.makeMilestone(
@@ -199,8 +285,6 @@
         collector = QueryCollector()
         collector.register()
         self.addCleanup(collector.unregister)
-        # This page is rather high in queries : 46 as a baseline. 20100817
-        page_query_limit = 46
         browser.open(milestone_url)
         # Check that the test found the bug
         self.assertTrue(bug1_url in browser.contents)
@@ -228,3 +312,127 @@
         self.assertEqual(with_1_private_bug, with_3_private_bugs,
             "different query count: \n%s\n******************\n%s\n" % (
             '\n'.join(with_1_queries), '\n'.join(with_3_queries)))
+
+
+class TestProjectGroupMilestoneIndexQueryCount(TestQueryCountBase):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestProjectGroupMilestoneIndexQueryCount, self).setUp()
+        # Increase cache size so that the query counts aren't affected
+        # by objects being removed from the cache early.
+        config.push('storm-cache', dedent('''
+            [launchpad]
+            storm_cache_size: 1000
+            '''))
+        self.addCleanup(config.pop, 'storm-cache')
+        self.owner = self.factory.makePerson(name='product-owner')
+        self.project_group = self.factory.makeProject(owner=self.owner)
+        login_person(self.owner)
+        self.milestone_name = 'foo'
+        # A ProjectGroup milestone doesn't exist unless one of its
+        # Projects has a milestone of that name.
+        product = self.factory.makeProduct(
+            owner=self.owner, project=self.project_group)
+        self.product_milestone = self.factory.makeMilestone(
+            productseries=product.development_focus,
+            name=self.milestone_name)
+        self.milestone = self.project_group.getMilestone(
+            self.milestone_name)
+
+    def add_bug(self, count):
+        login_person(self.owner)
+        for i in range(count):
+            bug = self.factory.makeBug(product=self.product_milestone.product)
+            bug.bugtasks[0].transitionToMilestone(
+                self.product_milestone, self.owner)
+            # This is necessary to test precaching of assignees.
+            bug.bugtasks[0].transitionToAssignee(
+                self.factory.makePerson())
+        logout()
+
+    def test_bugtasks_queries(self):
+        # The view.bugtasks attribute will make five queries:
+        #  1. For each project in the group load all the dev focus series ids.
+        #  2. Load bugtasks and bugs.
+        #  3. Load assignees (Person, Account, and EmailAddress).
+        #  4. Load links to specifications.
+        #  5. Load links to branches.
+        bugtask_count = 10
+        self.assert_bugtasks_query_count(
+            self.milestone, bugtask_count, query_limit=6)
+
+    def test_milestone_eager_loading(self):
+        # Verify that the number of queries does not increase with more
+        # bugs with different assignees as long as the number of
+        # projects doesn't increase.
+        query_limit = 37
+        self.add_bug(1)
+        self.assert_milestone_page_query_count(
+            self.milestone, query_limit=query_limit)
+        self.add_bug(10)
+        self.assert_milestone_page_query_count(
+            self.milestone, query_limit=query_limit)
+
+
+class TestDistributionMilestoneIndexQueryCount(TestQueryCountBase):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestDistributionMilestoneIndexQueryCount, self).setUp()
+        # Increase cache size so that the query counts aren't affected
+        # by objects being removed from the cache early.
+        config.push('storm-cache', dedent('''
+            [launchpad]
+            storm_cache_size: 1000
+            '''))
+        self.addCleanup(config.pop, 'storm-cache')
+        self.ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        self.owner = self.factory.makePerson(name='test-owner')
+        login_team(self.ubuntu.owner)
+        self.ubuntu.owner = self.owner
+        self.sourcepackagename = self.factory.makeSourcePackageName(
+            'foo-package')
+        login_person(self.owner)
+        self.milestone = self.factory.makeMilestone(
+            distribution=self.ubuntu)
+
+    def add_bug(self, count):
+        login_person(self.owner)
+        for i in range(count):
+            bug = self.factory.makeBug(distribution=self.ubuntu)
+            distrosourcepackage = self.factory.makeDistributionSourcePackage(
+                distribution=self.ubuntu)
+            bug.bugtasks[0].transitionToTarget(distrosourcepackage)
+            bug.bugtasks[0].transitionToMilestone(
+                self.milestone, self.owner)
+            # This is necessary to test precaching of assignees.
+            bug.bugtasks[0].transitionToAssignee(
+                self.factory.makePerson())
+        logout()
+
+    def test_bugtasks_queries(self):
+        # The view.bugtasks attribute will make seven queries:
+        #  1. Load ubuntu.currentseries.
+        #  2. Check if the user is in the admin team.
+        #  3. Check if the user is in the owner of the admin team.
+        #  4. Load bugtasks and bugs.
+        #  5. Load assignees (Person, Account, and EmailAddress).
+        #  6. Load links to specifications.
+        #  7. Load links to branches.
+        bugtask_count = 10
+        self.assert_bugtasks_query_count(
+            self.milestone, bugtask_count, query_limit=8)
+
+    def test_milestone_eager_loading(self):
+        # Verify that the number of queries does not increase with more
+        # bugs with different assignees.
+        query_limit = 32
+        self.add_bug(3)
+        self.assert_milestone_page_query_count(
+            self.milestone, query_limit=query_limit)
+        self.add_bug(10)
+        self.assert_milestone_page_query_count(
+            self.milestone, query_limit=query_limit)

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-11-22 23:08:11 +0000
+++ lib/lp/registry/interfaces/person.py	2010-12-09 15:28:31 +0000
@@ -2073,6 +2073,24 @@
     def cacheBrandingForPeople(people):
         """Prefetch Librarian aliases and content for personal images."""
 
+    def getPrecachedPersonsFromIDs(
+        person_ids, need_karma=False, need_ubuntu_coc=False,
+        need_location=False, need_archive=False,
+        need_preferred_email=False, need_validity=False):
+        """Lookup person objects from ids with optional precaching.
+
+        :param person_ids: List of person ids.
+        :param need_karma: The karma attribute will be cached.
+        :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
+            cached.
+        :param need_location: The location attribute will be cached.
+        :param need_archive: The archive attribute will be cached.
+        :param need_preferred_email: The preferred email attribute will be
+            cached.
+        :param need_validity: The is_valid attribute will be cached.
+        """
+
+
 
 class IRequestPeopleMerge(Interface):
     """This schema is used only because we want a very specific vocabulary."""

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-12-03 16:33:03 +0000
+++ lib/lp/registry/model/person.py	2010-12-09 15:28:31 +0000
@@ -1473,7 +1473,6 @@
         # Since we've updated the database behind Storm's back,
         # flush its caches.
         store.invalidate()
-        
 
         # Remove all indirect TeamParticipation entries resulting from this
         # team. If this were just a select, it would be a complicated but
@@ -1669,7 +1668,6 @@
         #       The difference between the two is that
         #       getMembersWithPreferredEmails includes self, which is arguably
         #       wrong, but perhaps deliberate.
-        store = Store.of(self)
         origin = [
             Person,
             Join(TeamParticipation, TeamParticipation.person == Person.id),
@@ -1679,99 +1677,12 @@
             TeamParticipation.team == self.id,
             # But not the team itself.
             TeamParticipation.person != self.id)
-        columns = [Person]
-        decorators = []
-        if need_karma:
-            # New people have no karmatotalcache rows.
-            origin.append(
-                LeftJoin(KarmaTotalCache,
-                    KarmaTotalCache.person == Person.id))
-            columns.append(KarmaTotalCache)
-        if need_ubuntu_coc:
-            columns.append(Alias(Exists(Select(SignedCodeOfConduct,
-                And(Person._is_ubuntu_coc_signer_condition(),
-                    SignedCodeOfConduct.ownerID == Person.id))),
-                name='is_ubuntu_coc_signer'))
-        if need_location:
-            # New people have no location rows
-            origin.append(
-                LeftJoin(PersonLocation,
-                    PersonLocation.person == Person.id))
-            columns.append(PersonLocation)
-        if need_archive:
-            # Not everyone has PPA's
-            # It would be nice to cleanly expose the soyuz rules for this to
-            # avoid duplicating the relationships.
-            origin.append(
-                LeftJoin(Archive, Archive.owner == Person.id))
-            columns.append(Archive)
-            conditions = And(conditions,
-                Or(Archive.id == None, And(
-                Archive.id == Select(Min(Archive.id),
-                    Archive.owner == Person.id, Archive),
-                Archive.purpose == ArchivePurpose.PPA)))
-        # checking validity requires having a preferred email.
-        if need_preferred_email and not need_validity:
-            # Teams don't have email, so a left join
-            origin.append(
-                LeftJoin(EmailAddress, EmailAddress.person == Person.id))
-            columns.append(EmailAddress)
-            conditions = And(conditions,
-                Or(EmailAddress.status == None,
-                    EmailAddress.status == EmailAddressStatus.PREFERRED))
-        if need_validity:
-            valid_stuff = Person._validity_queries()
-            origin.extend(valid_stuff["joins"])
-            columns.extend(valid_stuff["tables"])
-            decorators.extend(valid_stuff["decorators"])
-        if len(columns) == 1:
-            columns = columns[0]
-            # Return a simple ResultSet
-            return store.using(*origin).find(columns, conditions)
-        # Adapt the result into a cached Person.
-        columns = tuple(columns)
-        raw_result = store.using(*origin).find(columns, conditions)
-
-        def prepopulate_person(row):
-            result = row[0]
-            cache = get_property_cache(result)
-            index = 1
-            #-- karma caching
-            if need_karma:
-                karma = row[index]
-                index += 1
-                if karma is None:
-                    karma_total = 0
-                else:
-                    karma_total = karma.karma_total
-                cache.karma = karma_total
-            #-- ubuntu code of conduct signer status caching.
-            if need_ubuntu_coc:
-                signed = row[index]
-                index += 1
-                cache.is_ubuntu_coc_signer = signed
-            #-- location caching
-            if need_location:
-                location = row[index]
-                index += 1
-                cache.location = location
-            #-- archive caching
-            if need_archive:
-                archive = row[index]
-                index += 1
-                cache.archive = archive
-            #-- preferred email caching
-            if need_preferred_email and not need_validity:
-                email = row[index]
-                index += 1
-                cache.preferredemail = email
-            for decorator in decorators:
-                column = row[index]
-                index += 1
-                decorator(result, column)
-            return result
-        return DecoratedResultSet(raw_result,
-            result_decorator=prepopulate_person)
+        return PersonSet()._getPrecachedPersons(
+            origin, conditions, store=Store.of(self),
+            need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc,
+            need_location=need_location, need_archive=need_archive,
+            need_preferred_email=need_preferred_email,
+            need_validity=need_validity)
 
     def _getMembersWithPreferredEmails(self):
         """Helper method for public getMembersWithPreferredEmails.
@@ -4126,6 +4037,137 @@
         list(LibraryFileAlias.select("LibraryFileAlias.id IN %s"
              % sqlvalues(aliases), prejoins=["content"]))
 
+    def getPrecachedPersonsFromIDs(
+        self, person_ids, need_karma=False, need_ubuntu_coc=False,
+        need_location=False, need_archive=False,
+        need_preferred_email=False, need_validity=False):
+        """See `IPersonSet`."""
+        origin = [Person]
+        conditions = [
+            Person.id.is_in(person_ids)]
+        return self._getPrecachedPersons(
+            origin, conditions,
+            need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc,
+            need_location=need_location, need_archive=need_archive,
+            need_preferred_email=need_preferred_email,
+            need_validity=need_validity)
+
+    def _getPrecachedPersons(
+        self, origin, conditions, store=None,
+        need_karma=False, need_ubuntu_coc=False,
+        need_location=False, need_archive=False, need_preferred_email=False,
+        need_validity=False):
+        """Lookup all members of the team with optional precaching.
+
+        :param store: Provide ability to specify the store.
+        :param origin: List of storm tables and joins. This list will be
+            appended to. The Person table is required.
+        :param conditions: Storm conditions for tables in origin.
+        :param need_karma: The karma attribute will be cached.
+        :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
+            cached.
+        :param need_location: The location attribute will be cached.
+        :param need_archive: The archive attribute will be cached.
+        :param need_preferred_email: The preferred email attribute will be
+            cached.
+        :param need_validity: The is_valid attribute will be cached.
+        """
+        if store is None:
+            store = IStore(Person)
+        columns = [Person]
+        decorators = []
+        if need_karma:
+            # New people have no karmatotalcache rows.
+            origin.append(
+                LeftJoin(KarmaTotalCache,
+                    KarmaTotalCache.person == Person.id))
+            columns.append(KarmaTotalCache)
+        if need_ubuntu_coc:
+            columns.append(Alias(Exists(Select(SignedCodeOfConduct,
+                And(Person._is_ubuntu_coc_signer_condition(),
+                    SignedCodeOfConduct.ownerID == Person.id))),
+                name='is_ubuntu_coc_signer'))
+        if need_location:
+            # New people have no location rows
+            origin.append(
+                LeftJoin(PersonLocation,
+                    PersonLocation.person == Person.id))
+            columns.append(PersonLocation)
+        if need_archive:
+            # Not everyone has PPA's
+            # It would be nice to cleanly expose the soyuz rules for this to
+            # avoid duplicating the relationships.
+            origin.append(
+                LeftJoin(Archive, Archive.owner == Person.id))
+            columns.append(Archive)
+            conditions = And(conditions,
+                Or(Archive.id == None, And(
+                Archive.id == Select(Min(Archive.id),
+                    Archive.owner == Person.id, Archive),
+                Archive.purpose == ArchivePurpose.PPA)))
+        # checking validity requires having a preferred email.
+        if need_preferred_email and not need_validity:
+            # Teams don't have email, so a left join
+            origin.append(
+                LeftJoin(EmailAddress, EmailAddress.person == Person.id))
+            columns.append(EmailAddress)
+            conditions = And(conditions,
+                Or(EmailAddress.status == None,
+                    EmailAddress.status == EmailAddressStatus.PREFERRED))
+        if need_validity:
+            valid_stuff = Person._validity_queries()
+            origin.extend(valid_stuff["joins"])
+            columns.extend(valid_stuff["tables"])
+            decorators.extend(valid_stuff["decorators"])
+        if len(columns) == 1:
+            columns = columns[0]
+            # Return a simple ResultSet
+            return store.using(*origin).find(columns, conditions)
+        # Adapt the result into a cached Person.
+        columns = tuple(columns)
+        raw_result = store.using(*origin).find(columns, conditions)
+
+        def prepopulate_person(row):
+            result = row[0]
+            cache = get_property_cache(result)
+            index = 1
+            #-- karma caching
+            if need_karma:
+                karma = row[index]
+                index += 1
+                if karma is None:
+                    karma_total = 0
+                else:
+                    karma_total = karma.karma_total
+                cache.karma = karma_total
+            #-- ubuntu code of conduct signer status caching.
+            if need_ubuntu_coc:
+                signed = row[index]
+                index += 1
+                cache.is_ubuntu_coc_signer = signed
+            #-- location caching
+            if need_location:
+                location = row[index]
+                index += 1
+                cache.location = location
+            #-- archive caching
+            if need_archive:
+                archive = row[index]
+                index += 1
+                cache.archive = archive
+            #-- preferred email caching
+            if need_preferred_email and not need_validity:
+                email = row[index]
+                index += 1
+                cache.preferredemail = email
+            for decorator in decorators:
+                column = row[index]
+                index += 1
+                decorator(result, column)
+            return result
+        return DecoratedResultSet(raw_result,
+            result_decorator=prepopulate_person)
+
 
 # Provide a storm alias from Person to Owner. This is useful in queries on
 # objects that have more than just an owner associated with them.

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-11-30 22:01:15 +0000
+++ lib/lp/registry/tests/test_person.py	2010-12-09 15:28:31 +0000
@@ -67,6 +67,7 @@
     login_person,
     logout,
     person_logged_in,
+    StormStatementRecorder,
     TestCase,
     TestCaseWithFactory,
     )
@@ -428,12 +429,12 @@
         self.assertEqual('(\\u0170-tester)>', displayname)
 
 
-class TestPersonSet(TestCase):
+class TestPersonSet(TestCaseWithFactory):
     """Test `IPersonSet`."""
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        TestCase.setUp(self)
+        super(TestPersonSet, self).setUp()
         login(ANONYMOUS)
         self.addCleanup(logout)
         self.person_set = getUtility(IPersonSet)
@@ -457,6 +458,31 @@
             "PersonSet.getByEmail() should ignore case and whitespace.")
         self.assertEqual(person1, person2)
 
+    def test_getPrecachedPersonsFromIDs(self):
+        # The getPrecachedPersonsFromIDs() method should only make one
+        # query to load all the extraneous data. Accessing the
+        # attributes should then cause zero queries.
+        person_ids = [
+            self.factory.makePerson().id
+            for i in range(3)]
+
+        with StormStatementRecorder() as recorder:
+            persons = list(self.person_set.getPrecachedPersonsFromIDs(
+                person_ids, need_karma=True, need_ubuntu_coc=True,
+                need_location=True, need_archive=True,
+                need_preferred_email=True, need_validity=True))
+        self.assertThat(recorder, HasQueryCount(LessThan(2)))
+
+        with StormStatementRecorder() as recorder:
+            for person in persons:
+                person.is_valid_person
+                person.karma
+                person.is_ubuntu_coc_signer
+                person.location
+                person.archive
+                person.preferredemail
+        self.assertThat(recorder, HasQueryCount(LessThan(1)))
+
 
 class KarmaTestMixin:
     """Helper methods for setting karma."""