launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/bugtask-index-queries-726370 into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #726370 in Launchpad itself: "BugTask:+index timeout - late evaluation of Person/ValidPersonCache"
  https://bugs.launchpad.net/launchpad/+bug/726370

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bugtask-index-queries-726370/+merge/128850

== 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

-- 
https://code.launchpad.net/~wallyworld/launchpad/bugtask-index-queries-726370/+merge/128850
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bugtask-index-queries-726370 into lp:launchpad.

=== 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 02:49:21 +0000
@@ -809,10 +809,23 @@
             '(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)
+        cached_people = {}
+        for person in getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            person_ids, need_validity=True):
+            cached_people[person.id] = person
+
         interesting_activity = tuple(
-            BugActivityItem(activity)
-            for activity in activity
-            if interesting_match(activity.whatchanged) is not None)
+            BugActivityItem(
+                activity_item, cached_people[activity_item.personID])
+            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
@@ -4277,10 +4290,16 @@
 
 class BugActivityItem:
     """A decorated BugActivity."""
+
+    # `person` needs to be declared as a class attribute so that this class
+    # will not delegate the actual instance variables to self.activity,
+    # which would bypass the caching.
+    person = None
     delegates(IBugActivity, 'activity')
 
-    def __init__(self, activity):
+    def __init__(self, activity, activity_person):
         self.activity = activity
+        self.person = activity_person
 
     @property
     def change_summary(self):

=== 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 02:49: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

=== modified file 'lib/lp/bugs/doc/bugactivity.txt'
--- lib/lp/bugs/doc/bugactivity.txt	2011-08-01 05:25:59 +0000
+++ lib/lp/bugs/doc/bugactivity.txt	2012-10-10 02:49:21 +0000
@@ -271,7 +271,7 @@
     >>> activity = getUtility(IBugActivitySet).new(
     ...     bug=bug_one, whatchanged='summary', oldvalue='Old value',
     ...     newvalue='New value', person=user, datechanged=nowish)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
 
 The BugActivityItem offers properties that can be used to render the
 activity sensibly in an HTML interface. In most cases it just returns
@@ -304,7 +304,7 @@
     ...     bug=bug_one, whatchanged='security vulnerability',
     ...     oldvalue='no', newvalue='yes', person=user,
     ...     datechanged=nowish)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
 
     >>> print activity_item.change_details
     no → yes
@@ -312,7 +312,7 @@
     >>> activity = getUtility(IBugActivitySet).new(
     ...     bug=bug_one, whatchanged='visibility', oldvalue='public',
     ...     newvalue='private', person=user, datechanged=nowish)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
 
     >>> print activity_item.change_details
     public → private
@@ -323,7 +323,7 @@
     >>> activity = getUtility(IBugActivitySet).new(
     ...     bug=bug_one, whatchanged='tags', oldvalue='tag1 tag2',
     ...     newvalue='tag1 tag3', person=user, datechanged=nowish)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
     >>> print activity_item._formatted_tags_change
     added: tag3
     removed: tag2
@@ -341,7 +341,7 @@
     >>> activity = getUtility(IBugActivitySet).new(
     ...     bug=bug_one, whatchanged='malone: status', oldvalue='New',
     ...     newvalue='Triaged', person=user, datechanged=nowish)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
 
     >>> print activity_item.change_summary
     status
@@ -357,7 +357,7 @@
     >>> activity = getUtility(IBugActivitySet).new(
     ...     bug=bug_one, whatchanged='malone: assignee', oldvalue=None,
     ...     newvalue='somebody', person=user, datechanged=nowish)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
     >>> print activity_item.change_details
     nobody → somebody
 
@@ -365,7 +365,7 @@
     ...     bug=bug_one, whatchanged='malone: assignee',
     ...     oldvalue='somebody', newvalue=None, person=user,
     ...     datechanged=nowish)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
     >>> print activity_item.change_details
     somebody → nobody
 
@@ -376,7 +376,7 @@
     ...     bug=bug_one, whatchanged='description',
     ...     oldvalue='Old description', newvalue='New description',
     ...     person=user, datechanged=nowish)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
     >>> print "%s: %s" % (
     ...     activity_item.change_summary, activity_item.change_details)
     description: updated

=== modified file 'lib/lp/bugs/doc/bugcomment.txt'
--- lib/lp/bugs/doc/bugcomment.txt	2012-02-01 15:26:32 +0000
+++ lib/lp/bugs/doc/bugcomment.txt	2012-10-10 02:49:21 +0000
@@ -346,7 +346,7 @@
     ...     bug=bug_task.bug, whatchanged='malone: status',
     ...     oldvalue='New', newvalue='Confirmed',
     ...     person=user, datechanged=message.datecreated)
-    >>> activity_item = BugActivityItem(activity)
+    >>> activity_item = BugActivityItem(activity, user)
 
     >>> bug_comment = BugComment(
     ...     index=0, message=message, bugtask=bug_task,