launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1056617 into lp:launchpad.

Commit message:
Fix Milestone.specifications to be fast even when querying workitems.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1056617 in Launchpad itself: "ProductSeries:+index timing out due to workitems"
  https://bugs.launchpad.net/launchpad/+bug/1056617

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1056617/+merge/127412

Milestone.specifications has been rather slow (~150ms) since workitem filtering was added a few months back. This branch rewords it to use a more indexable (<10ms) UNION, and rewrites ProjectMilestone and ProjectGroupMilestoneTag's implementations to use the new common fast query.

It needs a new index on specification(milestone) to be really fast, but it's still substantially faster without it.

A test failure exposed a bug in the old query. If a blueprint had a deleted workitem targeted to the milestone, it would not be included in the result even if the blueprint itself was milestoned.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1056617/+merge/127412
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1056617 into lp:launchpad.

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2012-08-07 02:31:56 +0000
+++ lib/lp/registry/model/milestone.py	2012-10-02 04:47:19 +0000
@@ -28,9 +28,9 @@
 from storm.expr import (
     And,
     Desc,
-    Join,
     LeftJoin,
-    Or,
+    Select,
+    Union,
     )
 from storm.locals import Store
 from storm.zope import IResultSet
@@ -148,7 +148,36 @@
 
     @property
     def specifications(self):
-        raise NotImplementedError
+        from lp.registry.model.person import Person
+        store = Store.of(self.target)
+        origin = [
+            Specification,
+            LeftJoin(Person, Specification.assigneeID == Person.id),
+            ]
+        milestones = self._milestone_ids_expr
+
+        results = store.using(*origin).find(
+            (Specification, Person),
+            Specification.id.is_in(
+                Union(
+                    Select(
+                        Specification.id, tables=[Specification],
+                        where=(Specification.milestoneID.is_in(milestones))),
+                    Select(
+                        SpecificationWorkItem.specification_id,
+                        tables=[SpecificationWorkItem],
+                        where=And(
+                            SpecificationWorkItem.milestone_id.is_in(
+                                milestones),
+                            SpecificationWorkItem.deleted == False)),
+                    all=True)))
+        results.config(distinct=True)
+        ordered_results = results.order_by(Desc(Specification.priority),
+                                           Specification.definition_status,
+                                           Specification.implementation_status,
+                                           Specification.title)
+        mapper = lambda row: row[0]
+        return DecoratedResultSet(ordered_results, mapper)
 
     def bugtasks(self, user):
         """The list of non-conjoined bugtasks targeted to this milestone."""
@@ -190,30 +219,8 @@
     code_name = StringCol(dbName='codename', notNull=False, default=None)
 
     @property
-    def specifications(self):
-        from lp.registry.model.person import Person
-        store = Store.of(self)
-        origin = [
-            Specification,
-            LeftJoin(
-                SpecificationWorkItem,
-                SpecificationWorkItem.specification_id == Specification.id),
-            LeftJoin(Person, Specification.assigneeID == Person.id),
-            ]
-
-        results = store.using(*origin).find(
-            (Specification, Person),
-            Or(Specification.milestoneID == self.id,
-               SpecificationWorkItem.milestone_id == self.id),
-            Or(SpecificationWorkItem.deleted == None,
-               SpecificationWorkItem.deleted == False))
-        results.config(distinct=True)
-        ordered_results = results.order_by(Desc(Specification.priority),
-                                           Specification.definition_status,
-                                           Specification.implementation_status,
-                                           Specification.title)
-        mapper = lambda row: row[0]
-        return DecoratedResultSet(ordered_results, mapper)
+    def _milestone_ids_expr(self):
+        return (self.id,)
 
     @property
     def target(self):
@@ -421,36 +428,15 @@
         self.summary = None
 
     @property
-    def specifications(self):
-        """See `IMilestoneData`."""
-        from lp.registry.model.person import Person
+    def _milestone_ids_expr(self):
         from lp.registry.model.product import Product
-        store = Store.of(self.target)
-        origin = [
-            Specification,
-            LeftJoin(
-                SpecificationWorkItem,
-                SpecificationWorkItem.specification_id == Specification.id),
-            Join(Milestone,
-                 Or(Milestone.id == Specification.milestoneID,
-                    Milestone.id == SpecificationWorkItem.milestone_id)),
-            Join(Product, Product.id == Milestone.productID),
-            LeftJoin(Person, Specification.assigneeID == Person.id),
-            ]
-
-        results = store.using(*origin).find(
-            (Specification, Person),
-            Product.projectID == self.target.id,
-            Milestone.name == self.name,
-            Or(SpecificationWorkItem.deleted == None,
-               SpecificationWorkItem.deleted == False))
-        results.config(distinct=True)
-        ordered_results = results.order_by(Desc(Specification.priority),
-                                           Specification.definition_status,
-                                           Specification.implementation_status,
-                                           Specification.title)
-        mapper = lambda row: row[0]
-        return DecoratedResultSet(ordered_results, mapper)
+        return Select(
+            Milestone.id,
+            tables=[Milestone, Product],
+            where=And(
+                Milestone.name == self.name,
+                Milestone.productID == Product.id,
+                Product.project == self.target))
 
     @property
     def displayname(self):

=== modified file 'lib/lp/registry/model/milestonetag.py'
--- lib/lp/registry/model/milestonetag.py	2012-09-28 06:25:44 +0000
+++ lib/lp/registry/model/milestonetag.py	2012-10-02 04:47:19 +0000
@@ -10,27 +10,25 @@
     ]
 
 
-from storm.locals import (
+from storm.expr import (
+    And,
+    Exists,
+    Select,
+    )
+from storm.properties import (
     DateTime,
     Int,
-    Reference,
     Unicode,
     )
-from zope.component import getUtility
+from storm.references import Reference
 from zope.interface import implements
 
-from lp.blueprints.model.specification import Specification
 from lp.registry.interfaces.milestonetag import IProjectGroupMilestoneTag
 from lp.registry.model.milestone import (
     Milestone,
     MilestoneData,
     )
 from lp.registry.model.product import Product
-from lp.services.database.interfaces import (
-    DEFAULT_FLAVOR,
-    IStoreSelector,
-    MAIN_STORE,
-    )
 
 
 class MilestoneTag(object):
@@ -80,20 +78,19 @@
         return self.displayname
 
     @property
-    def specifications(self):
-        """See IMilestoneData."""
-        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        results = []
-        for tag in self.tags:
-            result = store.find(
-                Specification,
-                Specification.milestone == Milestone.id,
-                Milestone.product == Product.id,
+    def _milestone_ids_expr(self):
+        tag_constraints = And(*[
+            Exists(
+                Select(
+                    1, tables=[MilestoneTag],
+                    where=And(
+                        MilestoneTag.milestone_id == Milestone.id,
+                        MilestoneTag.tag == tag)))
+            for tag in self.tags])
+        return Select(
+            Milestone.id,
+            tables=[Milestone, Product],
+            where=And(
+                Milestone.productID == Product.id,
                 Product.project == self.target,
-                MilestoneTag.milestone_id == Milestone.id,
-                MilestoneTag.tag == tag)
-            results.append(result)
-        result = results.pop()
-        for i in results:
-            result = result.intersection(i)
-        return result
+                tag_constraints))

=== modified file 'lib/lp/registry/tests/test_milestone.py'
--- lib/lp/registry/tests/test_milestone.py	2012-07-09 08:48:31 +0000
+++ lib/lp/registry/tests/test_milestone.py	2012-10-02 04:47:19 +0000
@@ -201,16 +201,14 @@
         return specification, target_milestone
 
     def test_milestones_on_product(self):
-        specification, target_milestone = self._create_milestones_on_target(
+        spec, target_milestone = self._create_milestones_on_target(
             product=self.factory.makeProduct())
-        self.assertEqual([specification],
-                         list(target_milestone.specifications))
+        self.assertContentEqual([spec], target_milestone.specifications)
 
     def test_milestones_on_distribution(self):
-        specification, target_milestone = self._create_milestones_on_target(
+        spec, target_milestone = self._create_milestones_on_target(
             distribution=self.factory.makeDistribution())
-        self.assertEqual([specification],
-                         list(target_milestone.specifications))
+        self.assertContentEqual([spec], target_milestone.specifications)
 
     def test_milestones_on_project(self):
         # A Project (Project Group) milestone contains all specifications
@@ -218,11 +216,10 @@
         # a certain name.
         projectgroup = self.factory.makeProject()
         product = self.factory.makeProduct(project=projectgroup)
-        specification, target_milestone = self._create_milestones_on_target(
+        spec, target_milestone = self._create_milestones_on_target(
             product=product)
         milestone = projectgroup.getMilestone(name=target_milestone.name)
-        self.assertEqual([specification],
-                         list(milestone.specifications))
+        self.assertContentEqual([spec], milestone.specifications)
 
     def test_milestones_with_deleted_workitems(self):
         # Deleted work items do not cause the specification to show up
@@ -230,7 +227,7 @@
         milestone = self.factory.makeMilestone(
             product=self.factory.makeProduct())
         specification = self.factory.makeSpecification(
-            milestone=milestone, product=milestone.product)
+            product=milestone.product)
         self.factory.makeSpecificationWorkItem(
             specification=specification, milestone=milestone, deleted=True)
-        self.assertEqual([], list(milestone.specifications))
+        self.assertContentEqual([], milestone.specifications)