Merge lp:~stevenk/launchpad/no-more-o-n-queries-bug-dupes into lp:launchpad

[Steve Kowalik]

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

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.

[William Grant (wgrant)]

8 - # XXX 2010-10-05 gmb bug=655597:
9 - # This line of code keeps the view's query count down,
10 - # possibly using witchcraft. It should be rewritten to be
11 - # useful or removed in favour of making other queries more
12 - # efficient. The witchcraft is because the subscribers are accessed
13 - # in the initial page load, so the data is actually used.
14 - if self.user is not None:
15 - list(bug.getSubscribersForPerson(self.user))

I'd suggest restoring this for now. We know the rest of the changes are probably safe, but about this we're just guessing. It's not detrimental to leave, since we deferred the plan to redesign the method.

153 tzinfo=pytz.UTC)
154 +
155 +def generate_subscription_with(bug, person):

Looks like a missing newline.

155 +def generate_subscription_with(bug, person):
156 + return [
157 + With('all_bugsubscriptions', Select(
158 + (BugSubscription.id, BugSubscription.person_id),
159 + tables=[BugSubscription, Join(
160 + Bug, Bug.id == BugSubscription.bug_id)],
161 + where=Or(Bug.id == bug.id, Bug.duplicateofID == bug.id))),
162 + With('bugsubscriptions', Select(
163 + SQL('all_bugsubscriptions.id'),
164 + tables=[
165 + SQL('all_bugsubscriptions'), Join(
166 + TeamParticipation, TeamParticipation.teamID == SQL(
167 + 'all_bugsubscriptions.person'))],
168 + where=[TeamParticipation.personID == person.id]))]

Some of the wrapping here is a bit off.

274 + pillar = self._getTaskPillar(task)

Can you just use task.pillar instead?

292 + is_muted = Store.of(person).find(
293 + BugMute, BugMute.bug == bug, BugMute.person == person).one()
294 + return is_muted is not None

This could even be 'return not Store.of(person).find(BugMute, bug=bug, person=person).is_empty()'

review: Approve (code)

=== 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 11:59:29 +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/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-10 02:45:36 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-12 11:59:29 +0000
@@ -156,7 +156,7 @@
     def test_rendered_query_counts_constant_with_attachments(self):
         with celebrity_logged_in('admin'):
             browses_under_limit = BrowsesWithQueryLimit(
-                95, self.factory.makePerson())
+                96, self.factory.makePerson())
 
             # First test with a single attachment.
             task = self.factory.makeBugTask()

=== 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 11:59:29 +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,19 @@
     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 11:59:29 +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
 
@@ -19,17 +20,17 @@
     IVirtualSubscriptionInfo,
     IVirtualSubscriptionInfoCollection,
     )
-from lp.bugs.interfaces.structuralsubscription import (
-    IStructuralSubscriptionTargetHelper,
-    )
 from lp.bugs.model.bug import (
     Bug,
     BugMute,
+    generate_subscription_with,
     )
 from lp.bugs.model.bugsubscription import BugSubscription
-from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.registry.interfaces.person import IPersonSet
+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 +97,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 +119,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)
@@ -158,34 +159,17 @@
         """See `IPersonSubscriptions`."""
         self.loadSubscriptionsFor(self.person, self.bug)
 
-    def _getTaskPillar(self, bugtask):
-        """Return a pillar for a given BugTask."""
-        # There is no adaptor for ISourcePackage. Perhaps there
-        # should be since the data model doesn't seem to prohibit it.
-        # For now, we simply work around the problem.  It Would Be Nice If
-        # there were a reliable generic way of getting the pillar for any
-        # bugtarget, but we are not going to tackle that right now.
-        if ISourcePackage.providedBy(bugtask.target):
-            pillar = IStructuralSubscriptionTargetHelper(
-                bugtask.target.distribution_sourcepackage).pillar
-        else:
-            pillar = IStructuralSubscriptionTargetHelper(
-                bugtask.target).pillar
-        return pillar
-
     def _getDirectAndDuplicateSubscriptions(self, person, bug):
         # 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 +186,27 @@
                 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.
+            duplicates.annotateBugTaskResponsibilities(
+                task, task.pillar, task.pillar.bug_supervisor)
+            direct.annotateBugTaskResponsibilities(
+                task, task.pillar, task.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
+        return not Store.of(person).find(
+            BugMute, bug=bug, person=person).is_empty()
 
     def loadSubscriptionsFor(self, person, bug):
         self.person = person
@@ -246,13 +227,12 @@
         as_assignee = VirtualSubscriptionInfoCollection(
             self.person, self.administrated_team_ids)
         for bugtask in bug.bugtasks:
-            pillar = self._getTaskPillar(bugtask)
-            owner = pillar.owner
-            if person.inTeam(owner) and pillar.bug_supervisor is None:
-                as_owner.add(owner, bug, pillar, bugtask)
+            owner = bugtask.pillar.owner
+            if person.inTeam(owner) and bugtask.pillar.bug_supervisor is None:
+                as_owner.add(owner, bug, bugtask.pillar, bugtask)
             assignee = bugtask.assignee
             if person.inTeam(assignee):
-                as_assignee.add(assignee, bug, pillar, bugtask)
+                as_assignee.add(assignee, bug, bugtask.pillar, bugtask)
         self.count = 0
         for name, collection in (
             ('direct', direct), ('from_duplicate', from_duplicate),

=== modified file 'lib/lp/bugs/model/tests/test_personsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_personsubscriptioninfo.py	2012-08-21 04:04:47 +0000
+++ lib/lp/bugs/model/tests/test_personsubscriptioninfo.py	2012-10-12 11:59:29 +0000
@@ -502,7 +502,7 @@
         with StormStatementRecorder() as recorder:
             self.subscriptions.reload()
         # This should produce a very small number of queries.
-        self.assertThat(recorder, HasQueryCount(LessThan(5)))
+        self.assertThat(recorder, HasQueryCount(LessThan(6)))
         count_with_one_subscribed_duplicate = recorder.count
         # It should have the correct result.
         self.assertCollectionsAreEmpty(except_='from_duplicate')