launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/recipe-daily-build-performance-2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #711072 RootObject:+daily-builds times out
  https://bugs.launchpad.net/bugs/711072

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/recipe-daily-build-performance-2/+merge/51896

A further performance improvement for the +daily-builds page.

== Implementation ==
The page template had tales expressions like:

dailybuild/recipe/fmt:url
dailybuild/recipe/name

This caused the zope security infrastructure to do a permission check when navigating the path expression, which in turn caused getRelatedBranches() on each recipe to be called one by one, which in turn resulted in approx 3 queries per recipe. So the query count was always > 3 * nr recipes and for a batch size of 50, this meant that > 150 queries were executed, adding 4 or 5 seconds to the page render time. 

The fix was to provide the required recipe url and name values as properties on the view model objects, removing the trigger for a zope permission check.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/configure.zcml
  lib/lp/code/browser/tests/test_recipebuildslisting.py
  lib/lp/code/model/recipebuild.py
  lib/lp/code/templates/daily-builds-listing.pt

-- 
https://code.launchpad.net/~wallyworld/launchpad/recipe-daily-build-performance-2/+merge/51896
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/recipe-daily-build-performance-2 into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_recipebuildslisting.py'
--- lib/lp/code/browser/tests/test_recipebuildslisting.py	2011-03-01 03:30:19 +0000
+++ lib/lp/code/browser/tests/test_recipebuildslisting.py	2011-03-02 13:33:52 +0000
@@ -8,10 +8,12 @@
 
 from zope.component import getUtility
 
+from testtools.matchers import LessThan
+
 from canonical.launchpad.testing.pages import (
     extract_text,
     find_tag_by_id,
-    )
+    setupBrowserForUser)
 from canonical.launchpad.webapp.interfaces import ILaunchpadRoot
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.testing.layers import DatabaseFunctionalLayer
@@ -21,6 +23,8 @@
     login,
     TestCaseWithFactory,
     )
+from lp.testing._webservice import QueryCollector
+from lp.testing.matchers import HasQueryCount
 from lp.testing.views import create_initialized_view
 
 
@@ -90,6 +94,21 @@
         # Ensure we can see the listing when we are logged in.
         self._test_recipebuild_listing()
 
+    def test_recipebuild_listing_querycount(self):
+        # The query count on the recipe build listing page is small enough.
+        # There's a base query count of approx 30, but if the page template
+        # is not set up right, the query count can increases linearly with the
+        # number of records.
+        self.factory.makeRecipeBuildRecords(5, 0)
+        root = getUtility(ILaunchpadRoot)
+        url = canonical_url(root, view_name='+daily-builds', rootsite='code')
+        browser = setupBrowserForUser(self.user)
+        collector = QueryCollector()
+        collector.register()
+        browser.open(url)
+        counter = HasQueryCount(LessThan(35))
+        self.assertThat(collector, counter)
+
     def test_recipebuild_url(self):
         # Check the browser URL is as expected.
         root_url = self.layer.appserver_root_url(facet='code')

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2011-02-02 04:24:45 +0000
+++ lib/lp/code/configure.zcml	2011-03-02 13:33:52 +0000
@@ -1007,7 +1007,8 @@
 
   <class class="lp.code.model.recipebuild.RecipeBuildRecord">
     <allow attributes="sourcepackagename recipeowner recipe archive
-                        most_recent_build_time distro_source_package"/>
+                        most_recent_build_time distro_source_package
+                        recipe_name recipe_url"/>
   </class>
 
   <class class="lp.code.model.recipebuild.RecipeBuildRecordResultSet">

=== modified file 'lib/lp/code/model/recipebuild.py'
--- lib/lp/code/model/recipebuild.py	2011-03-01 03:30:19 +0000
+++ lib/lp/code/model/recipebuild.py	2011-03-02 13:33:52 +0000
@@ -31,6 +31,7 @@
     DecoratedResultSet,
     )
 from canonical.launchpad.interfaces.lpstorm import ISlaveStore
+from canonical.launchpad.webapp.publisher import canonical_url
 
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
@@ -73,6 +74,14 @@
         return self.archive.distribution.getSourcePackage(
             self.sourcepackagename)
 
+    @property
+    def recipe_name(self):
+        return self.recipe.name
+
+    @property
+    def recipe_url(self):
+        return canonical_url(self.recipe, rootsite='code')
+
 
 class RecipeBuildRecordSet:
     """See `IRecipeBuildRecordSet`."""

=== modified file 'lib/lp/code/templates/daily-builds-listing.pt'
--- lib/lp/code/templates/daily-builds-listing.pt	2011-01-13 23:10:07 +0000
+++ lib/lp/code/templates/daily-builds-listing.pt	2011-03-02 13:33:52 +0000
@@ -77,8 +77,8 @@
               </td>
 
               <td>
-                <a href="recipe" tal:attributes="href dailybuild/recipe/fmt:url">
-                <span tal:replace="dailybuild/recipe/name">
+                <a href="recipe" tal:attributes="href dailybuild/recipe_url">
+                <span tal:replace="dailybuild/recipe_name">
                   recipe
                 </span>
                 </a>