Merge lp:~wallyworld/launchpad/bugtask-index-queries-726370 into lp:launchpad

[Ian Booth]

Precache all bug activity persons to avoid individual queries on bugtask index page.

== Implementation ==

The bug activities rendered on the bugtask page were being iterated one by one and the activity person loaded. This caused 2 queries per activity person - one to select from Person and the other to select from ValidCache. This branches caches all activity people with a single query to prevent the linear growth in queries for this bit of the index page rendering.

== Tests ==

Add a new query count test for bugtasks with activities.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/doc/bugactivity.txt
  lib/lp/bugs/doc/bugcomment.txt

[William Grant (wgrant)]

As discussed on IRC, most of the invasive changes are unnecessary. Since it's a FK to the PK, you just need to materialise the resultset from getPrecachedPersonsFromIDs. Other than that this is great.

review: Approve (code)

[Ian Booth (wallyworld)]

Good pickup, thanks

On Wed 10 Oct 2012 13:12:18 EST, William Grant wrote:
> Review: Approve code
>
> As discussed on IRC, most of the invasive changes are unnecessary. Since it's a FK to the PK, you just need to materialise the resultset from getPrecachedPersonsFromIDs. Other than that this is great.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-10-08 09:29:02 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-10-10 03:14:21 +0000
@@ -809,10 +809,19 @@
             '(assignee|importance|milestone|status)')
         interesting_match = re.compile(
             "^(%s|%s)$" % (bug_change_re, bugtask_change_re)).match
+
+        activity_items = [
+            activity_item for activity_item in activity
+            if interesting_match(activity_item.whatchanged) is not None]
+        # Pre-load the doers of the activities in one query.
+        person_ids = set(
+            activity_item.personID for activity_item in activity_items)
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            person_ids, need_validity=True))
+
         interesting_activity = tuple(
-            BugActivityItem(activity)
-            for activity in activity
-            if interesting_match(activity.whatchanged) is not None)
+            BugActivityItem(activity_item) for activity_item in activity_items)
+
         # This is a bit kludgy but it means that interesting_activity is
         # populated correctly for all subsequent calls.
         self._interesting_activity_cached_value = interesting_activity

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-08 21:53:40 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-10 03:14:21 +0000
@@ -239,6 +239,31 @@
         [activity] = view.interesting_activity
         self.assertEqual("description", activity.whatchanged)
 
+    def test_rendered_query_counts_constant_with_activities(self):
+        # More queries are not used for extra bug activities.
+        task = self.factory.makeBugTask()
+
+        def add_activity(what, who):
+            getUtility(IBugActivitySet).new(
+                task.bug, datetime.now(UTC), who, whatchanged=what)
+
+        # Render the view with one activity.
+        with celebrity_logged_in('admin'):
+            browses_under_limit = BrowsesWithQueryLimit(
+                98, self.factory.makePerson())
+            person = self.factory.makePerson()
+            add_activity("description", person)
+
+        self.assertThat(task, browses_under_limit)
+
+        # Render the view with many more activities by different people.
+        with celebrity_logged_in('admin'):
+            for _ in range(20):
+                person = self.factory.makePerson()
+                add_activity("description", person)
+
+        self.assertThat(task, browses_under_limit)
+
     def test_error_for_changing_target_with_invalid_status(self):
         # If a user moves a bug task with a restricted status (say,
         # Triaged) to a target where they do not have permission to set