launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/launchpad/builders-timeout-bug-887078-eager-load into lp:launchpad with lp:~rvb/launchpad/builders-timeout-bug-887078-use-getSpecificJobs as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #887078 in Launchpad itself: "Builder:+history timeout"
  https://bugs.launchpad.net/launchpad/+bug/887078

For more details, see:
https://code.launchpad.net/~rvb/launchpad/builders-timeout-bug-887078-eager-load/+merge/82008

The ultimate goal is to make the number of queries issued by builder:+history independent of the number of builds.  We have 3 types of builds displayed on these pages: BinaryPackageBuild, SourcePackageRecipeBuild and TranslationTemplatesBuild.

This branch focuses on the elements of type BinaryPackageBuild and SourcePackageRecipeBuild. It eager loads data when the build farm jobs are fetched by getByBuildFarmJobs (BinaryPackageBuildSet and SourcePackageRecipeBuild).

- BinaryPackageBuild: the number of queries is now independent of the number of BinaryPackageBuilds \o/.
- SourcePackageRecipeBuild: I've improved things seriously here but each new SourcePackageRecipeBuild displayed generates 2 additional queries: this is because I've no idea how to prefetch what SourcePackageRecipe._recipe_data fetches. Also note that the test test_build_history_queries_count_view_recipe_builds does not tests the entire page generation but only the data preparation that is done by the view class. If we manage to cache SourcePackageRecipe._recipe_data somehow then more work could be done to make the whole page rendering issue a constant number of queries.

= Tests =

./bin/test -vvc test_builder_views test_build_history_queries_count_view_recipe_builds
./bin/test -vvc test_builder_views test_build_history_queries_count_binary_package_builds

= Q/A =

None.
-- 
https://code.launchpad.net/~rvb/launchpad/builders-timeout-bug-887078-eager-load/+merge/82008
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/builders-timeout-bug-887078-eager-load into lp:launchpad.

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2011-11-11 18:34:45 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2011-11-11 18:34:50 +0000
@@ -35,6 +35,9 @@
 
 from canonical.database.constants import UTC_NOW
 from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet,
+    )
 from canonical.launchpad.interfaces.lpstorm import (
     IMasterStore,
     IStore,
@@ -63,10 +66,17 @@
 from lp.code.mail.sourcepackagerecipebuild import (
     SourcePackageRecipeBuildMailer,
     )
+from lp.code.model.branch import Branch
 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.model.person import Person
+from lp.services.database.bulk import (
+    load_referencing,
+    load_related,
+    )
 from lp.services.job.model.job import Job
 from lp.soyuz.interfaces.archive import CannotUploadToArchive
+from lp.soyuz.model.archive import Archive
 from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
 from lp.soyuz.model.buildfarmbuildjob import BuildFarmBuildJob
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
@@ -289,9 +299,26 @@
             return EmptyResultSet()
         build_farm_job_ids = [
             build_farm_job.id for build_farm_job in build_farm_jobs]
-        return Store.of(build_farm_jobs[0]).find(cls,
+
+        def eager_load(rows):
+            # Circular imports.
+            from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
+            package_builds = load_related(
+                PackageBuild, rows, ['package_build_id'])
+            load_related(
+                Archive, package_builds, ['archive_id'])
+            sprs = load_related(
+                SourcePackageRecipe, rows, ['recipe_id'])
+            sprds = load_referencing(
+                SourcePackageRecipeData, sprs, ['sourcepackage_recipe_id'])
+            load_related(
+                Branch, sprds, ['base_branch_id'])
+            load_related(
+                Person, rows, ['requester_id'])
+        resultset = Store.of(build_farm_jobs[0]).find(cls,
             cls.package_build_id == PackageBuild.id,
             PackageBuild.build_farm_job_id.is_in(build_farm_job_ids))
+        return DecoratedResultSet(resultset, pre_iter_hook=eager_load)
 
     @classmethod
     def getRecentBuilds(cls, requester, recipe, distroseries, _now=None):

=== modified file 'lib/lp/soyuz/browser/tests/test_builder_views.py'
--- lib/lp/soyuz/browser/tests/test_builder_views.py	2011-11-11 18:34:45 +0000
+++ lib/lp/soyuz/browser/tests/test_builder_views.py	2011-11-11 18:34:50 +0000
@@ -7,6 +7,7 @@
 from testtools.matchers import Equals
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.database.sqlbase import flush_database_caches
 from canonical.launchpad.ftests import login
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import LaunchpadFunctionalLayer
@@ -62,33 +63,69 @@
 
     layer = LaunchpadFunctionalLayer
 
-    def createRecipeBuildWithBuilder(self, builder=None):
+    nb_objects = 2
+
+    def setUp(self):
+        super(TestBuilderHistoryView, self).setUp()
+        self.builder = self.factory.makeBuilder()
+
+    def createRecipeBuildWithBuilder(self):
         build = self.factory.makeSourcePackageRecipeBuild()
         Store.of(build).flush()
-        if builder is None:
-            builder = self.factory.makeBuilder()
-        removeSecurityProxy(build).builder = builder
-        return build
-
-    def test_build_history_queries_count(self):
-        # The number of queries issued by setupBuildList is not dependent
-        # on the number of builds.
-        builder = self.factory.makeBuilder()
-        self.createRecipeBuildWithBuilder(builder)
-        self.createRecipeBuildWithBuilder(builder)
-        # Record how many queries are issued when setupBuildList is
-        # called with 2 builds.
+        removeSecurityProxy(build).builder = self.builder
+        return build
+
+    def createBinaryPackageBuild(self):
+        build = self.factory.makeBinaryPackageBuild()
+        removeSecurityProxy(build).builder = self.builder
+        return build
+
+    def _record_queries_count(self, tested_method, item_creator):
+        # A simple helper that returns the two storm statement recorders
+        # obtained when running tested_method with {nb_objects} items creater
+        # (using item_creator) and then with {nb_objects}*2 items created.
+        for i in range(self.nb_objects):
+            item_creator()
+        # Record how many queries are issued when tested_method is
+        # called with {nb_objects} items created.
+        flush_database_caches()
         with StormStatementRecorder() as recorder1:
-            view = create_initialized_view(builder, '+history')
-            view.setupBuildList()
-            self.assertEqual(2, len(view.complete_builds))
-        # Create two more builds.
-        self.createRecipeBuildWithBuilder(builder)
-        self.createRecipeBuildWithBuilder(builder)
+            tested_method()
+        # Create {nb_objects} more items.
+        for i in range(self.nb_objects):
+            item_creator()
         # Record again the number of queries issued.
+        flush_database_caches()
         with StormStatementRecorder() as recorder2:
-            view = create_initialized_view(builder, '+history')
+            tested_method()
+        return recorder1, recorder2
+
+    def test_build_history_queries_count_view_recipe_builds(self):
+        # The builder's history view creation (i.e. the call to
+        # view.setupBuildList) issues a constant number of queries
+        # when recipe builds are displayed.
+        def call_setupBuildList():
+            view = create_initialized_view(self.builder, '+history')
             view.setupBuildList()
-            self.assertEqual(4, len(view.complete_builds))
+        recorder1, recorder2 = self._record_queries_count(
+            call_setupBuildList,
+            self.createRecipeBuildWithBuilder)
+
+        # rvb 2011-11-11: Each build issues 2 new queries.
+        # This is because of the way SourcePackageRecipe._recipe_data
+        # fetches SourcePackageRecipeData. I've no idea how to prefetch
+        # this.
+        self.assertThat(
+            recorder2,
+            HasQueryCount(Equals(recorder1.count + 2 * self.nb_objects)))
+
+    def test_build_history_queries_count_binary_package_builds(self):
+        # Rendering to builder's history issues a constant number of queries
+        # when binary builds are displayed.
+        def call_history_render():
+            create_initialized_view(self.builder, '+history').render()
+        recorder1, recorder2 = self._record_queries_count(
+            call_history_render,
+            self.createBinaryPackageBuild)
 
         self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2011-11-11 18:34:45 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2011-11-11 18:34:50 +0000
@@ -78,6 +78,7 @@
     PackageBuild,
     PackageBuildDerived,
     )
+from lp.services.database.bulk import load_related
 from lp.services.job.model.job import Job
 from lp.services.mail.sendmail import (
     format_address,
@@ -873,11 +874,45 @@
         clause_tables = (BinaryPackageBuild, PackageBuild, BuildFarmJob)
         build_farm_job_ids = [
             build_farm_job.id for build_farm_job in build_farm_jobs]
-        return Store.of(build_farm_jobs[0]).using(*clause_tables).find(
+
+        def eager_load(rows):
+            # Circular imports.
+            from lp.registry.model.sourcepackagename import (
+                SourcePackageName
+                )
+            from lp.soyuz.model.sourcepackagerelease import (
+                SourcePackageRelease
+                )
+            from lp.soyuz.model.distroarchseries import (
+                DistroArchSeries
+                )
+            from lp.registry.model.distroseries import (
+                DistroSeries
+                )
+            from lp.registry.model.distribution import (
+                Distribution
+                )
+            from lp.soyuz.model.archive import Archive
+            sprs = load_related(
+                SourcePackageRelease, rows, ['source_package_release_id'])
+            load_related(
+                SourcePackageName, sprs, ['sourcepackagenameID'])
+            distro_arch_series = load_related(
+                DistroArchSeries, rows, ['distro_arch_series_id'])
+            package_builds = load_related(
+                PackageBuild, rows, ['package_build_id'])
+            load_related(
+                Archive, package_builds, ['archive_id'])
+            distroseries = load_related(
+                DistroSeries, distro_arch_series, ['distroseriesID'])
+            load_related(
+                Distribution, distroseries, ['distributionID'])
+        resultset = Store.of(build_farm_jobs[0]).using(*clause_tables).find(
             BinaryPackageBuild,
             BinaryPackageBuild.package_build == PackageBuild.id,
             PackageBuild.build_farm_job == BuildFarmJob.id,
             BuildFarmJob.id.is_in(build_farm_job_ids))
+        return DecoratedResultSet(resultset, pre_iter_hook=eager_load)
 
     def getPendingBuildsForArchSet(self, archseries):
         """See `IBinaryPackageBuildSet`."""