launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-more-o-n-queries-bug-dupes into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #839141 in Launchpad itself: "Bug page times out getting subscriptions for logged in users on bugs with many duplicates"
  https://bugs.launchpad.net/launchpad/+bug/839141
  Bug #1062291 in Launchpad itself: "timeout on Bugtask:+Index in subscription lookups"
  https://bugs.launchpad.net/launchpad/+bug/1062291

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-more-o-n-queries-bug-dupes/+merge/129355

Rewrite the heavy-lifting queries in IBug.getSubscriptionForPerson() and IPersonSubscriptionInfo._getDirectAndDuplicateSubscriptions() to be performant using two CTEs. Also destroy the cause of excessive queries in the second function by bulk loading all related people, bugtasks and pillars.

Do a small number of whitespace function, and also rewrite IPersonSubscriptionInfo._isMuted() to be a shadow of it's former self. Drop the preloading browser.bugtask does with IBug.getSubscriptionForPerson(), but it may have to return.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-more-o-n-queries-bug-dupes/+merge/129355
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-more-o-n-queries-bug-dupes into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-10-10 03:12:08 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-10-12 06:28:20 +0000
@@ -739,15 +739,6 @@
             edit_url=canonical_url(self.context, view_name='+edit'),
             max_width='95%', truncate_lines=6)
 
-        # XXX 2010-10-05 gmb bug=655597:
-        # This line of code keeps the view's query count down,
-        # possibly using witchcraft. It should be rewritten to be
-        # useful or removed in favour of making other queries more
-        # efficient. The witchcraft is because the subscribers are accessed
-        # in the initial page load, so the data is actually used.
-        if self.user is not None:
-            list(bug.getSubscribersForPerson(self.user))
-
     def userIsSubscribed(self):
         """Is the user subscribed to this bug?"""
         return (

=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py	2012-09-28 16:47:42 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py	2012-10-12 06:28:20 +0000
@@ -269,8 +269,7 @@
             self.assertFalse(self.bug.isSubscribed(person))
             self.assertFalse(self.bug.isMuted(person))
             self.assertFalse(
-                self.bug.personIsAlsoNotifiedSubscriber(
-                    person))
+                self.bug.personIsAlsoNotifiedSubscriber(person))
             view = create_initialized_view(
                 self.bug, name="+portlet-subscription")
             self.assertFalse(view.user_should_see_mute_link)
@@ -305,14 +304,31 @@
             self.assertTrue(self.bug.isMuted(person))
             view = create_initialized_view(
                 self.bug, name="+portlet-subscription")
-            self.assertTrue(view.user_should_see_mute_link,
-                            "User should see mute link.")
+            self.assertTrue(
+                view.user_should_see_mute_link, "User should see mute link.")
             contents = view.render()
-            self.assertTrue('mute_subscription' in contents,
-                            "'mute_subscription' not in contents.")
+            self.assertTrue(
+                'mute_subscription' in contents,
+                "'mute_subscription' not in contents.")
             self.assertFalse(
-                self._hasCSSClass(
-                    contents, 'mute-link-container', 'hidden'))
+                self._hasCSSClass(contents, 'mute-link-container', 'hidden'))
+
+    def test_bug_portlet_subscription_query_count(self):
+        # Bug:+portlet-subscription doesn't make O(n) queries based on the
+        # number of duplicate bugs.
+        user = self.factory.makePerson()
+        bug = self.factory.makeBug()
+        for n in range(20):
+            dupe = self.factory.makeBug()
+            removeSecurityProxy(dupe)._markAsDuplicate(bug)
+            removeSecurityProxy(dupe).subscribe(user, dupe.owner)
+        Store.of(bug).invalidate()
+        with person_logged_in(user):
+            with StormStatementRecorder() as recorder:
+                view = create_initialized_view(
+                    bug, name='+portlet-subscription', principal=user)
+                view.render()
+        self.assertThat(recorder, HasQueryCount(Equals(21)))
 
 
 class TestBugSecrecyViews(TestCaseWithFactory):
@@ -328,12 +344,11 @@
         if bug is None:
             bug = self.factory.makeBug()
         with person_logged_in(person):
-            view = create_initialized_view(
+            return create_initialized_view(
                 bug.default_bugtask, name='+secrecy', form={
                     'field.information_type': 'USERDATA',
                     'field.actions.change': 'Change',
                     }, request=request)
-            return view
 
     def test_notification_shown_if_marking_private_and_not_subscribed(self):
         # If a user who is not subscribed to a bug marks that bug as

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-10-11 04:21:07 +0000
+++ lib/lp/bugs/model/bug.py	2012-10-12 06:28:20 +0000
@@ -13,6 +13,7 @@
     'BugSet',
     'BugTag',
     'FileBugData',
+    'generate_subscription_with',
     'get_also_notified_subscribers',
     'get_bug_tags_open_count',
     ]
@@ -54,6 +55,7 @@
     SQL,
     Sum,
     Union,
+    With,
     )
 from storm.info import ClassAlias
 from storm.locals import (
@@ -1056,36 +1058,21 @@
             else:
                 self._subscriber_dups_cache.add(subscriber)
             return subscriber
-        return DecoratedResultSet(Store.of(self).find(
+        with_statement = generate_subscription_with(self, person)
+        store = Store.of(self).with_(with_statement)
+        return DecoratedResultSet(store.find(
              # Return people and subscriptions
             (Person, BugSubscription),
-            # For this bug or its duplicates
-            Or(
-                Bug.id == self.id,
-                Bug.duplicateof == self.id),
-            # Get subscriptions for these bugs
-            BugSubscription.bug_id == Bug.id,
-            # Filter by subscriptions to any team person is in.
-            # Note that teamparticipation includes self-participation entries
-            # (person X is in the team X)
-            TeamParticipation.person == person.id,
-            # XXX: Storm fails to compile this, so manually done.
-            # bug=https://bugs.launchpad.net/storm/+bug/627137
-            # RBC 20100831
-            SQL("""TeamParticipation.team = BugSubscription.person"""),
-            # Join in the Person rows we want
-            # XXX: Storm fails to compile this, so manually done.
-            # bug=https://bugs.launchpad.net/storm/+bug/627137
-            # RBC 20100831
-            SQL("""Person.id = TeamParticipation.team"""),
+            BugSubscription.id.is_in(
+                SQL('SELECT bugsubscriptions.id FROM bugsubscriptions')),
+            Person.id == BugSubscription.person_id,
             ).order_by(Person.name).config(
                 distinct=(Person.name, BugSubscription.person_id)),
             cache_subscriber, pre_iter_hook=cache_unsubscribed)
 
     def getSubscriptionForPerson(self, person):
         """See `IBug`."""
-        store = Store.of(self)
-        return store.find(
+        return Store.of(self).find(
             BugSubscription,
             BugSubscription.person == person,
             BugSubscription.bug == self).one()
@@ -2830,3 +2817,18 @@
     date_created = DateTime(
         "date_created", allow_none=False, default=UTC_NOW,
         tzinfo=pytz.UTC)
+
+def generate_subscription_with(bug, person):
+    return [
+        With('all_bugsubscriptions', Select(
+            (BugSubscription.id, BugSubscription.person_id),
+            tables=[BugSubscription, Join(
+                Bug, Bug.id == BugSubscription.bug_id)],
+            where=Or(Bug.id == bug.id, Bug.duplicateofID == bug.id))),
+        With('bugsubscriptions', Select(
+            SQL('all_bugsubscriptions.id'),
+            tables=[
+                SQL('all_bugsubscriptions'), Join(
+                    TeamParticipation, TeamParticipation.teamID == SQL(
+                        'all_bugsubscriptions.person'))],
+            where=[TeamParticipation.personID == person.id]))]

=== modified file 'lib/lp/bugs/model/personsubscriptioninfo.py'
--- lib/lp/bugs/model/personsubscriptioninfo.py	2012-08-22 01:41:50 +0000
+++ lib/lp/bugs/model/personsubscriptioninfo.py	2012-10-12 06:28:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -6,8 +6,9 @@
     'PersonSubscriptions',
     ]
 
-from storm.expr import Or
+from storm.expr import SQL
 from storm.store import Store
+from zope.component import getUtility
 from zope.interface import implements
 from zope.proxy import sameProxiedObjects
 
@@ -25,11 +26,15 @@
 from lp.bugs.model.bug import (
     Bug,
     BugMute,
+    generate_subscription_with,
     )
 from lp.bugs.model.bugsubscription import BugSubscription
+from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.registry.model.distribution import Distribution
 from lp.registry.model.person import Person
-from lp.registry.model.teammembership import TeamParticipation
+from lp.registry.model.product import Product
+from lp.services.database.bulk import load_related
 
 
 class RealSubscriptionInfo:
@@ -96,8 +101,8 @@
             person, administrated_team_ids)
         self._principal_pillar_to_info = {}
 
-    def _add_item_to_collection(self,
-                                collection, principal, bug, pillar, task):
+    def _add_item_to_collection(self, collection, principal, bug, pillar,
+                                task):
         key = (principal, pillar)
         info = self._principal_pillar_to_info.get(key)
         if info is None:
@@ -118,8 +123,8 @@
             person, administrated_team_ids)
         self._principal_bug_to_infos = {}
 
-    def _add_item_to_collection(self, collection, principal,
-                                bug, subscription):
+    def _add_item_to_collection(self, collection, principal, bug,
+                                subscription):
         info = RealSubscriptionInfo(principal, bug, subscription)
         key = (principal, bug)
         infos = self._principal_bug_to_infos.get(key)
@@ -177,15 +182,13 @@
         # Fetch all information for direct and duplicate
         # subscriptions (including indirect through team
         # membership) in a single query.
-        store = Store.of(person)
-        bug_id_options = [Bug.id == bug.id, Bug.duplicateofID == bug.id]
-        info = store.find(
+        with_statement = generate_subscription_with(bug, person)
+        info = Store.of(person).with_(with_statement).find(
             (BugSubscription, Bug, Person),
-            BugSubscription.bug == Bug.id,
-            BugSubscription.person == Person.id,
-            Or(*bug_id_options),
-            TeamParticipation.personID == person.id,
-            TeamParticipation.teamID == Person.id)
+            BugSubscription.id.is_in(
+                SQL('SELECT bugsubscriptions.id FROM bugsubscriptions')),
+            Person.id == BugSubscription.person_id,
+            Bug.id == BugSubscription.bug_id)
 
         direct = RealSubscriptionInfoCollection(
             self.person, self.administrated_team_ids)
@@ -202,30 +205,30 @@
                 collection = direct
             collection.add(
                 subscriber, subscribed_bug, subscription)
+        # Preload bug owners, then all pillars.
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            [bug.ownerID for bug in bugs]))
+        all_tasks = [task for task in bug.bugtasks for bug in bugs] 
+        load_related(Product, all_tasks, ['productID'])
+        load_related(Distribution, all_tasks, ['distributionID'])
         for bug in bugs:
             # indicate the reporter and bug_supervisor
             duplicates.annotateReporter(bug, bug.owner)
             direct.annotateReporter(bug, bug.owner)
-            for task in bug.bugtasks:
-                # Get bug_supervisor.
-                pillar = self._getTaskPillar(task)
-                duplicates.annotateBugTaskResponsibilities(
-                    task, pillar, pillar.bug_supervisor)
-                direct.annotateBugTaskResponsibilities(
-                    task, pillar, pillar.bug_supervisor)
+        for task in all_tasks:
+            # Get bug_supervisor.
+            pillar = self._getTaskPillar(task)
+            duplicates.annotateBugTaskResponsibilities(
+                task, pillar, pillar.bug_supervisor)
+            direct.annotateBugTaskResponsibilities(
+                task, pillar, pillar.bug_supervisor)
         return (direct, duplicates)
 
     def _isMuted(self, person, bug):
         store = Store.of(person)
-        mutes = store.find(
-            BugMute,
-            BugMute.bug == bug,
-            BugMute.person == person)
-        is_muted = mutes.one()
-        if is_muted is None:
-            return False
-        else:
-            return True
+        is_muted = store.find(
+            BugMute, BugMute.bug == bug, BugMute.person == person).one()
+        return is_muted is not None
 
     def loadSubscriptionsFor(self, person, bug):
         self.person = person