launchpad-reviewers team mailing list archive

[Colin Watson <mp+450917@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:optimize-determineFilesToSend into launchpad:master.

Commit message:
Optimize BinaryPackageBuildBehaviour.determineFilesToSend

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/450917

This eliminates two database queries for every file that forms part of a source package.  Not a lot, but this is part of buildd-manager's scan cycle so removing some unnecessary round-trips seems worthwhile.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:optimize-determineFilesToSend into launchpad:master.

diff --git a/lib/lp/soyuz/model/sourcepackagerelease.py b/lib/lp/soyuz/model/sourcepackagerelease.py
index 6edb2fe..6e1222e 100644
--- a/lib/lp/soyuz/model/sourcepackagerelease.py
+++ b/lib/lp/soyuz/model/sourcepackagerelease.py
@@ -18,7 +18,7 @@ from debian.changelog import (
     ChangelogCreateError,
     ChangelogParseError,
 )
-from storm.expr import Coalesce, Join, Sum
+from storm.expr import Coalesce, Join, LeftJoin, Sum
 from storm.locals import DateTime, Desc, Int, Reference, Unicode
 from storm.store import Store
 from zope.component import getUtility
@@ -361,11 +361,32 @@ class SourcePackageRelease(StormBase):
     @cachedproperty
     def files(self):
         """See `ISourcePackageRelease`."""
-        return list(
-            Store.of(self)
-            .find(SourcePackageReleaseFile, sourcepackagerelease=self)
+        # Preload library files, since call sites will normally need them too.
+        return [
+            sprf
+            for sprf, _, _ in Store.of(self)
+            .using(
+                SourcePackageReleaseFile,
+                Join(
+                    LibraryFileAlias,
+                    SourcePackageReleaseFile.libraryfile
+                    == LibraryFileAlias.id,
+                ),
+                LeftJoin(
+                    LibraryFileContent,
+                    LibraryFileAlias.content == LibraryFileContent.id,
+                ),
+            )
+            .find(
+                (
+                    SourcePackageReleaseFile,
+                    LibraryFileAlias,
+                    LibraryFileContent,
+                ),
+                SourcePackageReleaseFile.sourcepackagerelease == self,
+            )
             .order_by(SourcePackageReleaseFile.libraryfile_id)
-        )
+        ]
 
     def getFileByName(self, filename):
         """See `ISourcePackageRelease`."""
diff --git a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
index ee8278e..c7eaf0d 100644
--- a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
@@ -45,6 +45,7 @@ from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.services.authserver.testing import InProcessAuthServerFixture
 from lp.services.config import config
+from lp.services.database.sqlbase import flush_database_caches
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.log.logger import BufferLogger
 from lp.services.macaroons.testing import MacaroonVerifies
@@ -53,11 +54,12 @@ from lp.services.webapp import canonical_url
 from lp.soyuz.adapters.archivedependencies import get_sources_list_for_building
 from lp.soyuz.enums import ArchivePurpose, PackagePublishingStatus
 from lp.soyuz.tests.soyuz import Base64KeyMatches
-from lp.testing import TestCaseWithFactory
+from lp.testing import StormStatementRecorder, TestCaseWithFactory
 from lp.testing.dbuser import switch_dbuser
 from lp.testing.gpgkeys import gpgkeysdir
 from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import LaunchpadZopelessLayer, ZopelessDatabaseLayer
+from lp.testing.matchers import HasQueryCount
 
 
 class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
@@ -508,6 +510,34 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         self.assertTrue(extra_args["arch_indep"])
 
     @defer.inlineCallbacks
+    def test_determineFilesToSend_query_count(self):
+        build = self.factory.makeBinaryPackageBuild()
+        behaviour = IBuildFarmJobBehaviour(build)
+
+        def add_file(build):
+            build.source_package_release.addFile(
+                self.factory.makeLibraryFileAlias(db_only=True),
+                filetype=SourcePackageFileType.COMPONENT_ORIG_TARBALL,
+            )
+
+        # This is more or less `lp.testing.record_two_runs`, but that
+        # doesn't work with asynchronous code, and it's easy enough to
+        # inline the relevant bits.
+        for _ in range(2):
+            add_file(build)
+        flush_database_caches()
+        with StormStatementRecorder() as recorder1:
+            filemap = yield behaviour.determineFilesToSend()
+            self.assertEqual(2, len(list(filemap)))
+        for _ in range(2):
+            add_file(build)
+        flush_database_caches()
+        with StormStatementRecorder() as recorder2:
+            filemap = yield behaviour.determineFilesToSend()
+            self.assertEqual(4, len(list(filemap)))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+    @defer.inlineCallbacks
     def test_extraBuildArgs_archives_proposed(self):
         # A build in the primary archive's proposed pocket uses the release,
         # security, updates, and proposed pockets in the primary archive.