Merge lp:~lifeless/launchpad/getBuildSummariesForSourceIds into lp:launchpad

[Robert Collins]

Group up calls in Archive:EntryResource:getBuildSummariesForSourceIds rather than suffering death by a thousand queries.

Group up calls in Archive:EntryResource:getBuildSummariesForSourceIds rather than suffering death by a thousand queries. I haven't tuned the queries (which also needs doing), nor have I written a test for the query counts. For the former, I want the actual queries we should be doing to be visible, which is why I've refactored to a constant query count(ish). For the latter I haven't reached the activation energy needed to write one yet, and while I think it would be useful, I think the most useful thing to do right now is fix the OOPSes.

[Tim Penhey (thumper)]

I don't fully understand what is going on, but the code looks fine.

review: Approve

[Robert Collins (lifeless)]

Thanks Tim. I'm moving stuff from this pattern:
{{{
SQL
for thing in things:
   thing.method_doing_SQL
   thing.other_method_doing_SQL
}}}

to
{{{
SQL
SQL
SQL
post_process_in_python
}}}

In the single build case, same amount of SQL. In the 100 package case, vastly less ;)

=== modified file 'lib/lp/soyuz/adapters/archivesourcepublication.py'
--- lib/lp/soyuz/adapters/archivesourcepublication.py	2010-11-03 06:55:53 +0000
+++ lib/lp/soyuz/adapters/archivesourcepublication.py	2010-11-08 19:18:52 +0000
@@ -92,7 +92,8 @@
         # using the delegate as self - might not be possible without
         # duck-typing.
         return getUtility(
-            IPublishingSet).getBuildStatusSummaryForSourcePublication(self)
+            IPublishingSet).getBuildStatusSummaryForSourcePublication(self,
+            self.getBuilds)
 
 class ArchiveSourcePublications:
     """`ArchiveSourcePublication` iterator."""

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2010-10-06 12:40:00 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2010-11-08 19:18:52 +0000
@@ -507,6 +507,10 @@
         """Return a resultset of `IBuild` objects in this context that are
         not published.
 
+        Note that this is convenience glue for
+        PublishingSet.getUnpublishedBuildsForSources - and that method should
+        be considered authoritative.
+
         :param build_states: list of build states to which the result should
             be limited. Defaults to BuildStatus.FULLYBUILT if none are
             specified.
@@ -1075,7 +1079,8 @@
             `IBinaryPackagePublishingHistory`.
         """
 
-    def getBuildStatusSummariesForSourceIdsAndArchive(source_ids, archive):
+    def getBuildStatusSummariesForSourceIdsAndArchive(source_ids, archive,
+        get_builds=None):
         """Return a summary of the build statuses for source publishing ids.
 
         This method collects all the builds for the provided source package
@@ -1090,6 +1095,9 @@
         :param archive: The archive which will be used to filter the source
                         ids.
         :type archive: `IArchive`
+        :param get_builds: Optional override to replace database querying for
+            build records. This is used in the ArchivePublication adapter code
+            path and should be avoided where possible.
         :return: A dict consisting of the overall status summaries for the
             given ids that belong in the archive. For example:
                 {
@@ -1100,7 +1108,8 @@
         :rtype: ``dict``.
         """
 
-    def getBuildStatusSummaryForSourcePublication(source_publication):
+    def getBuildStatusSummaryForSourcePublication(source_publication,
+        get_builds=None):
         """Return a summary of the build statuses for this source
         publication.
 
@@ -1108,6 +1117,13 @@
         for details. The call is just proxied here so that it can also be
         used with an ArchiveSourcePublication passed in as
         the source_package_pub, allowing the use of the cached results.
+
+        :param get_builds: An optional callback to replace the database lookup
+            builds. This is used by some adapters as part of a preloading
+            strategy - but the preferred form is to have
+            getBuildStatusSummariesForSourceIdsAndArchive (which this call
+            delegates to) perform the initial and only loading of the build
+            records.
         """
 
     def getNearestAncestor(

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-11-08 03:45:09 +0000
+++ lib/lp/soyuz/model/publishing.py	2010-11-08 19:18:52 +0000
@@ -16,6 +16,7 @@
     ]
 
 
+from collections import defaultdict
 from datetime import datetime
 import operator
 import os
@@ -502,25 +503,23 @@
 
         return unique_binary_publications
 
+    @staticmethod
+    def _convertBuilds(builds_for_sources):
+        """Convert from IPublishingSet getBuilds to SPPH getBuilds."""
+        return [build for source, build, arch in builds_for_sources]
+
     def getBuilds(self):
         """See `ISourcePackagePublishingHistory`."""
         publishing_set = getUtility(IPublishingSet)
-        result_set = publishing_set.getBuildsForSources(self)
-
-        return [build for source, build, arch in result_set]
+        result_set = publishing_set.getBuildsForSources([self])
+        return SourcePackagePublishingHistory._convertBuilds(result_set)
 
     def getUnpublishedBuilds(self, build_states=None):
         """See `ISourcePackagePublishingHistory`."""
         publishing_set = getUtility(IPublishingSet)
         result_set = publishing_set.getUnpublishedBuildsForSources(
             self, build_states)
-
-        # Create a function that will just return the second item
-        # in the result tuple (the build).
-        def result_to_build(result):
-            return result[1]
-
-        return DecoratedResultSet(result_set, result_to_build)
+        return DecoratedResultSet(result_set, operator.itemgetter(1))
 
     def changesFileUrl(self):
         """See `ISourcePackagePublishingHistory`."""
@@ -1699,28 +1698,84 @@
             PackageUploadSource.sourcepackagereleaseID == spr.id)
         return result_set.one()
 
-    def getBuildStatusSummariesForSourceIdsAndArchive(self,
-                                                      source_ids,
-                                                      archive):
+    def getBuildStatusSummariesForSourceIdsAndArchive(self, source_ids,
+        archive, get_builds=None):
         """See `IPublishingSet`."""
         # source_ids can be None or an empty sequence.
         if not source_ids:
             return {}
 
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        source_pubs = store.find(
-            SourcePackagePublishingHistory,
-            SourcePackagePublishingHistory.id.is_in(source_ids),
-            SourcePackagePublishingHistory.archive == archive)
-
+        if len(source_ids) == 1 and get_builds is not None:
+            # Fake out the DB access for ArchivePublication
+            source_pubs = list(store.find(
+                SourcePackagePublishingHistory,
+                SourcePackagePublishingHistory.id.is_in(source_ids),
+                SourcePackagePublishingHistory.archive == archive))
+            source = source_pubs[0]
+            build_info = [(source, build, None) for build in get_builds()]
+        else:
+            build_info = list(
+                self.getBuildsForSourceIds(source_ids, archive=archive))
+            source_pubs = set()
+            found_source_ids = set()
+            for row in build_info:
+                source_pubs.add(row[0])
+                found_source_ids.add(row[0].id)
+            pubs_without_builds = set(source_ids) - found_source_ids
+            if pubs_without_builds:
+                # Add in source pubs for which no builds were found: we may in
+                # future want to make this a LEFT OUTER JOIN in
+                # getBuildsForSourceIds but to avoid destabilising other code
+                # paths while we fix performance, it is just done as a single
+                # separate query for now.
+                source_pubs.update(store.find(
+                    SourcePackagePublishingHistory,
+                    SourcePackagePublishingHistory.id.is_in(
+                        pubs_without_builds),
+                    SourcePackagePublishingHistory.archive == archive))
+        # For each source_pub found, provide an aggregate summary of its
+        # builds.
+        binarypackages = getUtility(IBinaryPackageBuildSet)
         source_build_statuses = {}
+        need_unpublished = set()
         for source_pub in source_pubs:
-            status_summary = source_pub.getStatusSummaryForBuilds()
-            source_build_statuses[source_pub.id] = status_summary
+            source_builds = [build for build in build_info 
+                if build[0].id == source_pub.id]
+            builds = SourcePackagePublishingHistory._convertBuilds(source_builds)
+            summary = binarypackages.getStatusSummaryForBuilds(builds)
+            source_build_statuses[source_pub.id] = summary
+
+            # If:
+            #   1. the SPPH is in an active publishing state, and
+            #   2. all the builds are fully-built, and
+            #   3. the SPPH is not being published in a rebuild/copy archive (in
+            #      which case the binaries are not published)
+            #   4. There are unpublished builds
+            # Then we augment the result with FULLYBUILT_PENDING and attach the
+            # unpublished builds.
+            if (source_pub.status in active_publishing_status and
+                    summary['status'] == BuildSetStatus.FULLYBUILT and
+                    not source_pub.archive.is_copy):
+                need_unpublished.add(source_pub)
+
+        if need_unpublished:
+            unpublished = list(self.getUnpublishedBuildsForSources(
+                need_unpublished))
+            unpublished_per_source = defaultdict(list)
+            for source_pub, build, _ in unpublished:
+                unpublished_per_source[source_pub].append(build)
+            for source_pub, builds in unpublished_per_source.items():
+                summary = {
+                    'status': BuildSetStatus.FULLYBUILT_PENDING,
+                    'builds': builds
+                }
+                source_build_statuses[source_pub.id] = summary
 
         return source_build_statuses
 
-    def getBuildStatusSummaryForSourcePublication(self, source_publication):
+    def getBuildStatusSummaryForSourcePublication(self, source_publication,
+        get_builds=None):
         """See `ISourcePackagePublishingHistory`.getStatusSummaryForBuilds.
 
         This is provided here so it can be used by both the SPPH as well
@@ -1728,31 +1783,9 @@
         the same interface but uses cached results for builds and binaries
         used in the calculation.
         """
-        builds = source_publication.getBuilds()
-        summary = getUtility(
-            IBinaryPackageBuildSet).getStatusSummaryForBuilds(builds)
-
-        # We only augment the result if:
-        #   1. we (the SPPH) are ourselves in an active publishing state, and
-        #   2. all the builds are fully-built, and
-        #   3. we are not being published in a rebuild/copy archive (in
-        #      which case the binaries are not currently published anyway)
-        # In this case we check to see if they are all published, and if
-        # not we return FULLYBUILT_PENDING:
-        augmented_summary = summary
-        if (source_publication.status in active_publishing_status and
-                summary['status'] == BuildSetStatus.FULLYBUILT and
-                not source_publication.archive.is_copy):
-
-            unpublished_builds = list(
-                source_publication.getUnpublishedBuilds())
-
-            if unpublished_builds:
-                augmented_summary = {
-                    'status': BuildSetStatus.FULLYBUILT_PENDING,
-                    'builds': unpublished_builds
-                }
-        return augmented_summary
+        source_id = source_publication.id
+        return self.getBuildStatusSummariesForSourceIdsAndArchive([source_id],
+            source_publication.archive, get_builds=get_builds)[source_id]
 
     def requestDeletion(self, sources, removed_by, removal_comment=None):
         """See `IPublishingSet`."""