Merge lp:~wallyworld/launchpad/projectgroup-timeout-1016156 into lp:launchpad

[Ian Booth]

Rework some entity properties to reduce query counts on the projectgroup index page and others.

== Implementation ==

The entity domain objects had (non-cached) properties which return result sets. So every time the property was referenced via a TAL expression, the query was re-run. I identified some core properties on various classes which are implicated on the project group index page, and also other pillar pages. These were converted to cached properties and listified. The result set creation was moved off to a separate getter if required.

As an example, for one project group index page, query count drops in half from the original number of about 70.

== Tests ==

A generic, internal optimisation, so rely on existing tests.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/answers/browser/faqcollection.py
  lib/lp/answers/browser/questiontarget.py
  lib/lp/blueprints/model/sprint.py
  lib/lp/registry/browser/announcement.py
  lib/lp/registry/browser/project.py
  lib/lp/registry/doc/projectgroup.txt
  lib/lp/registry/interfaces/projectgroup.py
  lib/lp/registry/model/projectgroup.py
  lib/lp/registry/templates/hasannouncements-portlet-latest.pt

[Curtis Hovey (sinzui)]

The two changes to /lib/answers looks wrong. I see that we are converting the result set to a list, thus instantiating about 100 FAQs, but we only return 5. The change to questiontarget will instantiate several 1000 questions in the U1 and launchpad project groups. Surely this is slower than how the code it currently written. I think the current codes is issue a single query with a LIMIT and instantiating just the 5 FAQs or questions wanted. I think we want them converted to a list *after* the slice.

review: Needs Information (code)

[Curtis Hovey (sinzui)]

Oh sorry for the conflicting review status. I don't want this branch to land until you and I agree about what is happening with the slicing in these two cached properties.

[Ian Booth (wallyworld)]

> The two changes to /lib/answers looks wrong. I see that we are converting the
> result set to a list, thus instantiating about 100 FAQs, but we only return 5.
> The change to questiontarget will instantiate several 1000 questions in the U1
> and launchpad project groups. Surely this is slower than how the code it
> currently written. I think the current codes is issue a single query with a
> LIMIT and instantiating just the 5 FAQs or questions wanted. I think we want
> them converted to a list *after* the slice.

Ah, good pickup. My fat fingers mistyped the final bracket. The 2 cases should be fixed now.

The philosophy I'm using is that all cached properties should return a list (not a generator or result set etc which is re-run each time) since this conflicts with the expected semantics that there is no work in calling the cached property a second time. Hence the listification of the result set.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'lib/lp/answers/browser/faqcollection.py'
--- lib/lp/answers/browser/faqcollection.py	2012-01-01 02:58:52 +0000
+++ lib/lp/answers/browser/faqcollection.py	2012-10-14 23:44:21 +0000
@@ -127,7 +127,7 @@
         quantity = 5
         faqs = self.context.searchFAQs(
             search_text=self.search_text, sort=FAQSort.NEWEST_FIRST)
-        return faqs[:quantity]
+        return list(faqs[:quantity])
 
     @safe_action
     @action(_('Search'), name='search')
 
=== modified file 'lib/lp/answers/browser/questiontarget.py'
--- lib/lp/answers/browser/questiontarget.py	2012-07-05 00:47:31 +0000
+++ lib/lp/answers/browser/questiontarget.py	2012-10-14 23:44:21 +0000
@@ -167,7 +167,7 @@
         is used by the +portlet-latestquestions view.
         """
         question_collection = IQuestionCollection(self.context)
-        return question_collection.searchQuestions()[:quantity]
+        return list(question_collection.searchQuestions()[:quantity])
 
 
 class QuestionCollectionOpenCountView:
 
=== modified file 'lib/lp/blueprints/doc/sprint.txt'
--- lib/lp/blueprints/doc/sprint.txt	2012-09-28 14:35:35 +0000
+++ lib/lp/blueprints/doc/sprint.txt	2012-10-14 23:44:21 +0000
@@ -91,7 +91,9 @@
 Flush the updates to the database so we'll see them.
 
     >>> from lp.services.database.sqlbase import flush_database_updates
+    >>> from lp.services.propertycache import clear_property_cache
     >>> flush_database_updates()
+    >>> clear_property_cache(ubuntu)
 
 See, there are no ubuntu sprints.
 
 
=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py	2012-09-27 18:50:38 +0000
+++ lib/lp/blueprints/model/sprint.py	2012-10-14 23:44:21 +0000
@@ -60,6 +60,7 @@
     SQLBase,
     )
 from lp.services.database.stormexpr import fti_search
+from lp.services.propertycache import cachedproperty
 
 
 class Sprint(SQLBase, HasDriversMixin, HasSpecificationsMixin):
@@ -361,22 +362,28 @@
                    quote(SprintSpecificationStatus.ACCEPTED))
         return query, ['Specification', 'SprintSpecification']
 
-    @property
+    def getSprints(self):
+        query, tables = self._getBaseQueryAndClauseTablesForQueryingSprints()
+        return Sprint.select(
+            query, clauseTables=tables, orderBy='-time_starts', distinct=True)
+
+    @cachedproperty
     def sprints(self):
         """See IHasSprints."""
-        query, tables = self._getBaseQueryAndClauseTablesForQueryingSprints()
-        return Sprint.select(
-            query, clauseTables=tables, orderBy='-time_starts', distinct=True)
+        return list(self.getSprints())
 
-    @property
-    def coming_sprints(self):
-        """See IHasSprints."""
+    def getComingSprings(self):
         query, tables = self._getBaseQueryAndClauseTablesForQueryingSprints()
         query += " AND Sprint.time_ends > 'NOW'"
         return Sprint.select(
             query, clauseTables=tables, orderBy='time_starts',
             distinct=True, limit=5)
 
+    @cachedproperty
+    def coming_sprints(self):
+        """See IHasSprints."""
+        return list(self.getComingSprings())
+
     @property
     def past_sprints(self):
         """See IHasSprints."""
 
=== modified file 'lib/lp/registry/browser/announcement.py'
--- lib/lp/registry/browser/announcement.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/announcement.py	2012-10-14 23:44:21 +0000
@@ -298,18 +298,22 @@
     @cachedproperty
     def announcements(self):
         published_only = not check_permission('launchpad.Edit', self.context)
-        return self.context.getAnnouncements(
-                    limit=None, published_only=published_only)
+        return list(self.context.getAnnouncements(
+                    limit=None, published_only=published_only))
 
     @cachedproperty
     def latest_announcements(self):
         published_only = not check_permission('launchpad.Edit', self.context)
-        return self.context.getAnnouncements(
-                    limit=5, published_only=published_only)
+        return list(self.context.getAnnouncements(
+                    limit=5, published_only=published_only))
+
+    @cachedproperty
+    def has_announcements(self):
+        return len(self.latest_announcements) > 0
 
     @cachedproperty
     def show_announcements(self):
-        return (self.latest_announcements.count() > 0
+        return (len(self.latest_announcements) > 0
             or check_permission('launchpad.Edit', self.context))
 
     @cachedproperty
 
=== modified file 'lib/lp/registry/browser/project.py'
--- lib/lp/registry/browser/project.py	2012-10-08 01:55:25 +0000
+++ lib/lp/registry/browser/project.py	2012-10-14 23:44:21 +0000
@@ -390,7 +390,7 @@
         The number of sub projects can break the preferred layout so the
         template may want to plan for a long list.
         """
-        return self.context.products.count() > 10
+        return len(self.context.products) > 10
 
     @property
     def project_group_milestone_tag(self):
 
=== modified file 'lib/lp/registry/browser/tests/pillar-views.txt'
--- lib/lp/registry/browser/tests/pillar-views.txt	2012-06-16 13:41:36 +0000
+++ lib/lp/registry/browser/tests/pillar-views.txt	2012-10-14 23:44:21 +0000
@@ -216,6 +216,8 @@
     ...     productseries=series,
     ...     language_codes=['es'])
     >>> product.translations_usage = ServiceUsage.LAUNCHPAD
+    >>> from lp.services.propertycache import clear_property_cache
+    >>> clear_property_cache(project_group)
     >>> project_group.has_translatable()
     True
 
 
=== modified file 'lib/lp/registry/browser/tests/projectgroup-views.txt'
--- lib/lp/registry/browser/tests/projectgroup-views.txt	2012-08-08 07:22:51 +0000
+++ lib/lp/registry/browser/tests/projectgroup-views.txt	2012-10-14 23:44:21 +0000
@@ -7,7 +7,7 @@
 
     >>> projectgroup = factory.makeProject(name='mothership')
     >>> view = create_view(projectgroup, name='+index')
-    >>> projectgroup.products.count()
+    >>> len(projectgroup.products)
     0
 
     >>> view.has_many_projects
@@ -23,7 +23,9 @@
     >>> owner = projectgroup.owner
     >>> ignored = login_person(owner)
     >>> add_daughter(projectgroup, 'a')
-    >>> projectgroup.products.count()
+    >>> from lp.services.propertycache import clear_property_cache
+    >>> clear_property_cache(projectgroup)
+    >>> len(projectgroup.products)
     1
 
     >>> view = create_view(projectgroup, name='+index')
@@ -34,7 +36,8 @@
 projects (10 projects are roughly 2 portlets deep.)
 
     >>> add_daughter(projectgroup, 'bcdefghijk')
-    >>> projectgroup.products.count()
+    >>> clear_property_cache(projectgroup)
+    >>> len(projectgroup.products)
     11
 
     >>> view = create_view(projectgroup, name='+index')
 
=== modified file 'lib/lp/registry/doc/projectgroup.txt'
--- lib/lp/registry/doc/projectgroup.txt	2012-09-26 20:56:43 +0000
+++ lib/lp/registry/doc/projectgroup.txt	2012-10-14 23:44:21 +0000
@@ -131,6 +131,8 @@
     >>> unlink_source_packages(netapplet)
     >>> netapplet.active = False
     >>> flush_database_updates()
+    >>> from lp.services.propertycache import clear_property_cache
+    >>> clear_property_cache(gnome)
     >>> [product.displayname for product in gnome.products]
     [u'Evolution', u'GNOME Terminal', u'Gnome Applets', u'gnomebaker']
 
@@ -356,8 +358,8 @@
     >>> gnome = getUtility(IProjectGroupSet)['gnome']
     >>> gnome.has_translatable()
     True
-    >>> translatables = gnome.translatables()
-    >>> translatables.count()
+    >>> translatables = gnome.translatables
+    >>> len(translatables)
     1
 
 And that translatable product is 'Evolution'.
 
=== modified file 'lib/lp/registry/interfaces/projectgroup.py'
--- lib/lp/registry/interfaces/projectgroup.py	2012-10-08 02:42:05 +0000
+++ lib/lp/registry/interfaces/projectgroup.py	2012-10-14 23:44:21 +0000
@@ -326,19 +326,14 @@
         title=u"Search for possible duplicate bugs when a new bug is filed",
         required=False, readonly=True)
 
+    translatables = Attribute("Products that are translatable in LP")
+
     def getProduct(name):
         """Get a product with name `name`."""
 
     def getConfigurableProducts():
         """Get all products that can be edited by user."""
 
-    def translatables():
-        """Return an iterator over products that are translatable in LP.
-
-        Only products with IProduct.translations_usage set to
-        ServiceUsage.LAUNCHPAD are considered translatable.
-        """
-
     def has_translatable():
         """Return a boolean showing the existance of translatables products.
         """
 
=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2012-10-08 02:42:05 +0000
+++ lib/lp/registry/model/projectgroup.py	2012-10-14 23:44:21 +0000
@@ -98,6 +98,7 @@
     sqlvalues,
     )
 from lp.services.helpers import shortlist
+from lp.services.propertycache import cachedproperty
 from lp.services.webapp.authorization import check_permission
 from lp.services.worlddata.model.language import Language
 from lp.translations.enums import TranslationPermission
@@ -168,11 +169,14 @@
         """See `IPillar`."""
         return "Project Group"
 
-    @property
-    def products(self):
+    def getProducts(self):
         return Product.selectBy(
             project=self, active=True, orderBy='displayname')
 
+    @cachedproperty
+    def products(self):
+        return list(self.getProducts())
+
     def getProduct(self, name):
         return Product.selectOneBy(project=self, name=name)
 
@@ -187,8 +191,12 @@
             return [self.driver]
         return []
 
-    def translatables(self):
-        """See `IProjectGroup`."""
+    def getTranslatables(self):
+        """Return an iterator over products that are translatable in LP.
+
+        Only products with IProduct.translations_usage set to
+        ServiceUsage.LAUNCHPAD are considered translatable.
+        """
         store = Store.of(self)
         origin = [
             Product,
@@ -201,9 +209,14 @@
             Product.translations_usage == ServiceUsage.LAUNCHPAD,
             ).config(distinct=True)
 
+    @cachedproperty
+    def translatables(self):
+        """See `IProjectGroup`."""
+        return list(self.getTranslatables())
+
     def has_translatable(self):
         """See `IProjectGroup`."""
-        return not self.translatables().is_empty()
+        return len(self.translatables) > 0
 
     def sharesTranslationsWithOtherSide(self, person, language,
                                         sourcepackage=None,
@@ -383,7 +396,7 @@
         This is to avoid situations where users try to file bugs against
         empty project groups (Malone bug #106523).
         """
-        return self.products.count() != 0
+        return len(self.products) != 0
 
     def _getMilestoneCondition(self):
         """See `HasMilestonesMixin`."""
 
=== modified file 'lib/lp/registry/templates/hasannouncements-portlet-latest.pt'
--- lib/lp/registry/templates/hasannouncements-portlet-latest.pt	2009-08-11 04:35:26 +0000
+++ lib/lp/registry/templates/hasannouncements-portlet-latest.pt	2012-10-14 23:44:21 +0000
@@ -4,7 +4,7 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
   omit-tag="">
 <tal:news define="announcements view/latest_announcements;
-                  has_announcements announcements/count;
+                  has_announcements view/has_announcements;
                   overview_menu context/menu:overview">
 <div id="portlet-latest-announcements" class="portlet announcements"
   tal:condition="view/show_announcements">
 
=== modified file 'lib/lp/translations/browser/project.py'
--- lib/lp/translations/browser/project.py	2012-02-28 04:24:19 +0000
+++ lib/lp/translations/browser/project.py	2012-10-14 23:44:21 +0000
@@ -52,7 +52,7 @@
 
     @property
     def untranslatables(self):
-        translatables = set(self.context.translatables())
+        translatables = set(self.context.translatables)
         all_products = set(self.context.products)
         return list(all_products - translatables)