Merge lp:~rvb/launchpad/builders-timeout-bug-887078-eager-load into lp:launchpad

[Raphaël Badin]

Eager load data in getByBuildFarmJobs.

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 fetch what SourcePackageRecipe._recipe_data fetches in bulk so that it's properly cached by Storm. 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.

[Gavin Panella (allenap)]

\o/

[1]

+ flush_database_updates()
         with StormStatementRecorder() as recorder:

I end up doing this every time. I'll put a branch in ec2 that calls
flush_database_updates() in StormStatementRecorder.__enter__().

[2]

+ build_farm_jobs.extend([build.build_farm_job for build in builds2])

No need for the intermediate list, though I guess it does little harm.

[3]

+ # 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.

I guess this is work in progress, but should probably be XXX'd
anyway. What do you think?

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review.

> I end up doing this every time. I'll put a branch in ec2 that calls
> flush_database_updates() in StormStatementRecorder.__enter__().

Good idea ;)

> [2]
>
> + build_farm_jobs.extend([build.build_farm_job for build in builds2])
>
> No need for the intermediate list, though I guess it does little harm.

Okay.

> [3]
>
> + # 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.
>
> I guess this is work in progress, but should probably be XXX'd
> anyway. What do you think?

I'll keep it as is but I'll work on a branch to fix this right away.

=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjob.py'
--- lib/lp/buildmaster/tests/test_buildfarmjob.py	2011-11-10 15:11:52 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjob.py	2011-11-14 09:09:41 +0000
@@ -248,11 +248,14 @@
         Store.of(sprb).flush()
         return sprb
 
+    def createBinaryPackageBuild(self):
+        build = self.factory.makeBinaryPackageBuild()
+        return build
+
     def createBuilds(self):
         builds = []
-        for i in xrange(10):
-            # We don't create binary package builds because the test
-            # would be really heavy to setup.
+        for i in xrange(2):
+            builds.append(self.createBinaryPackageBuild())
             builds.append(self.createTranslationTemplateBuild())
             builds.append(self.createSourcePackageRecipeBuild())
         return builds
@@ -278,13 +281,20 @@
             self.build_farm_job_set.getSpecificJobs([]))
 
     def test_getSpecificJobs_sql_queries_count(self):
-        # getSpecificJobs issues one query for each build type.
+        # getSpecificJobs issues a constant number of queries.
         builds = self.createBuilds()
         build_farm_jobs = [build.build_farm_job for build in builds]
+        flush_database_updates()
         with StormStatementRecorder() as recorder:
             self.build_farm_job_set.getSpecificJobs(
                 build_farm_jobs)
-        self.assertThat(recorder, HasQueryCount(Equals(2)))
+        builds2 = self.createBuilds()
+        build_farm_jobs.extend([build.build_farm_job for build in builds2])
+        flush_database_updates()
+        with StormStatementRecorder() as recorder2:
+            self.build_farm_job_set.getSpecificJobs(
+                build_farm_jobs)
+        self.assertThat(recorder, HasQueryCount(Equals(recorder2.count)))
 
     def test_getSpecificJobs_no_specific_job(self):
         build_farm_job_source = getUtility(IBuildFarmJobSource)

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2011-11-09 11:36:44 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2011-11-14 09:09:41 +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-10 15:14:00 +0000
+++ lib/lp/soyuz/browser/tests/test_builder_views.py	2011-11-14 09:09:41 +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 20:34:43 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2011-11-14 09:09:41 +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`."""