launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-618367 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #618367 Distribution:+assignments is taking a long time maybe 20% of the time
  https://bugs.launchpad.net/bugs/618367


Second stab at addressing /ubuntu/+assignments.

This branch does two things:
 - it uses a slightly different preloading strategy which avoids some sometimes slow stuff in storm 
 - it batches with a default of 500 (we have 3000 specs on that page)

This is one of the slowest pages we have now (\o/).

The only downside I can see here is that the sorted columns on the table are not brilliantly integrated into the batching, but I don't know of any workaround for that.
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-618367/+merge/35489
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-618367 into lp:launchpad/devel.

=== modified file 'lib/lp/blueprints/browser/specificationtarget.py'
--- lib/lp/blueprints/browser/specificationtarget.py	2010-08-24 10:45:57 +0000
+++ lib/lp/blueprints/browser/specificationtarget.py	2010-09-15 00:12:50 +0000
@@ -326,6 +326,12 @@
         return self.context.specifications(filter=filter)
 
     @cachedproperty
+    def specs_batched(self):
+        navigator = BatchNavigator(self.specs, self.request, size=500)
+        navigator.setHeadings('specification', 'specifications')
+        return navigator
+
+    @cachedproperty
     def spec_count(self):
         return self.specs.count()
 

=== modified file 'lib/lp/blueprints/browser/tests/test_specificationtarget.py'
--- lib/lp/blueprints/browser/tests/test_specificationtarget.py	2010-08-20 20:31:18 +0000
+++ lib/lp/blueprints/browser/tests/test_specificationtarget.py	2010-09-15 00:12:50 +0000
@@ -3,8 +3,8 @@
 
 __metaclass__ = type
 
-import unittest
-
+from canonical.launchpad.webapp.batching import BatchNavigator
+from canonical.launchpad.testing.pages import find_tag_by_id
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.blueprints.interfaces.specificationtarget import (
     IHasSpecifications,
@@ -15,7 +15,10 @@
     login_person,
     TestCaseWithFactory,
     )
-from lp.testing.views import create_view
+from lp.testing.views import (
+    create_view,
+    create_initialized_view,
+    )
 
 
 class TestRegisterABlueprintButtonView(TestCaseWithFactory):
@@ -86,12 +89,43 @@
         self.assertFalse(
             '<div id="involvement" class="portlet involvement">' in view())
 
-
-def test_suite():
-    suite = unittest.TestSuite()
-    suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
-    return suite
-
-
-if __name__ == '__main__':
-    unittest.TextTestRunner().run(test_suite())
+    def test_specs_batch(self):
+        # Some pages turn up in very large contexts and patch. E.g.
+        # Distro:+assignments which uses SpecificationAssignmentsView, a
+        # subclass.
+        person = self.factory.makePerson()
+        view = create_initialized_view(person, name='+assignments')
+        self.assertIsInstance(view.specs_batched, BatchNavigator)
+
+    def test_batch_headings(self):
+        person = self.factory.makePerson()
+        view = create_initialized_view(person, name='+assignments')
+        navigator = view.specs_batched
+        self.assertEqual('specification', navigator._singular_heading)
+        self.assertEqual('specifications', navigator._plural_heading)
+
+    def test_batch_size(self):
+        # Because +assignments is meant to provide an overview, we default to
+        # 500 as the default batch size.
+        person = self.factory.makePerson()
+        view = create_initialized_view(person, name='+assignments')
+        navigator = view.specs_batched
+        self.assertEqual(500, view.specs_batched.default_size)
+
+
+class TestAssignments(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_assignments_are_batched(self):
+        product = self.factory.makeProduct()
+        spec1 = self.factory.makeSpecification(product=product)
+        spec2 = self.factory.makeSpecification(product=product)
+        view = create_initialized_view(product, name='+assignments',
+            query_string="batch=1")
+        content = view.render()
+        self.assertEqual('next',
+            find_tag_by_id(content, 'upper-batch-nav-batchnav-next')['class'])
+        self.assertEqual('next',
+            find_tag_by_id(content, 'lower-batch-nav-batchnav-next')['class'])
+        

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2010-08-31 19:38:30 +0000
+++ lib/lp/blueprints/model/specification.py	2010-09-15 00:12:50 +0000
@@ -713,50 +713,39 @@
         """
         # Circular import.
         from lp.registry.model.person import Person
-        assignee = ClassAlias(Person, "assignee")
-        approver = ClassAlias(Person, "approver")
-        drafter = ClassAlias(Person, "drafter")
-        origin = [Specification,
-            LeftJoin(assignee, Specification.assignee==assignee.id),
-            LeftJoin(approver, Specification.approver==approver.id),
-            LeftJoin(drafter, Specification.drafter==drafter.id),
-            ]
-        columns = [Specification, assignee, approver, drafter]
-        for alias in (assignee, approver, drafter):
-            validity_info = Person._validity_queries(person_table=alias)
+        def cache_people(rows):
+            # Find the people we need:
+            person_ids = set()
+            for spec in rows:
+                person_ids.add(spec.assigneeID)
+                person_ids.add(spec.approverID)
+                person_ids.add(spec.drafterID)
+            person_ids.discard(None)
+            if not person_ids:
+                return
+            # Query those people
+            origin = [Person]
+            columns = [Person]
+            validity_info = Person._validity_queries()
             origin.extend(validity_info["joins"])
             columns.extend(validity_info["tables"])
-            # We only need one decorators list: all the decorators will be
-            # bound the same, and having a single list lets us more easily call
-            # the right one.
             decorators = validity_info["decorators"]
-        columns = tuple(columns)
-        results = Store.of(self).using(*origin).find(
-            columns,
+            personset = Store.of(self).using(*origin).find(
+                tuple(columns),
+                Person.id.is_in(person_ids),
+                )
+            for row in personset:
+                person = row[0]
+                index = 1
+                for decorator in decorators:
+                    column = row[index]
+                    index += 1
+                    decorator(person, column)
+        results = Store.of(self).find(
+            Specification,
             SQL(query),
             )
-        def cache_person(person, row, start_index):
-            """apply caching decorators to person."""
-            index = start_index
-            for decorator in decorators:
-                column = row[index]
-                index += 1
-                decorator(person, column)
-            return index
-        def cache_validity(row):
-            # Assignee
-            person = row[1]
-            # After drafter
-            index = 4
-            index = cache_person(person, row, index)
-            # approver
-            person = row[2]
-            index = cache_person(person, row, index)
-            # drafter
-            person = row[3]
-            index = cache_person(person, row, index)
-            return row[0]
-        return DecoratedResultSet(results, cache_validity)
+        return DecoratedResultSet(results, pre_iter_hook=cache_people)
 
     @property
     def valid_specifications(self):

=== modified file 'lib/lp/blueprints/templates/specificationtarget-assignments.pt'
--- lib/lp/blueprints/templates/specificationtarget-assignments.pt	2009-09-22 15:02:41 +0000
+++ lib/lp/blueprints/templates/specificationtarget-assignments.pt	2010-09-15 00:12:50 +0000
@@ -31,6 +31,7 @@
       sign off on the specification.
     </p>
 
+    <tal:navigation replace="structure view/specs_batched/@@+navigation-links-upper" />
     <table class="listing sortable" id="work">
       <thead>
         <tr>
@@ -43,8 +44,8 @@
           <th>Approver</th>
         </tr>
       </thead>
-      <tbody class="lesser">
-        <tr tal:repeat="spec view/specs">
+      <tbody class="lesser" tal:define="specs view/specs_batched/currentBatch">
+        <tr tal:repeat="spec specs">
           <td>
             <span class="sortkey" tal:content="spec/priority/sortkey" />
             <span tal:content="spec/priority/title"
@@ -91,6 +92,7 @@
         </tr>
       </tbody>
     </table>
+    <tal:navigation replace="structure view/specs_batched/@@+navigation-links-lower" />
   </div>
 
 </div>

=== modified file 'lib/lp/services/database/prejoin.py'
--- lib/lp/services/database/prejoin.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/database/prejoin.py	2010-09-15 00:12:50 +0000
@@ -1,7 +1,11 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Prejoin for Storm."""
+"""Prejoin for Storm.
+
+XXX bug=632150 This is duplicated with DecoratedResultSet. please use that.
+RBC 20100907.
+"""
 
 __metaclass__ = type
 __all__ = ['prejoin']