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

[Robert Collins]

[r=stub][ui=none][bug=672371] Sanify ppa/+packages to have constant query counts as source packages are added.

Fix scaling of ppa/+packages to be flat with source packages, distro series and binary packages. I'm not completely confident in the binary package angle as the page is driven by source packages - but its better than nothing.

The key change here is to move an adapter lookup into an eager loaded dataset - the older code which was a problem previously was exacerbated by the underlying changes made to optimise getBuildStatusSummariesForSourceIdsAndArchive. As a result of which I could strip out some code made just for that code path.

[Steve Kowalik (stevenk)]

Hi,

With in your test, you look to make a PPA, and then 5 lines later, make a different PPA, but assigned to the same variable, and *then* you make a 3rd PPA assigned to the same variable. This is confusing.

You also set a baseline variable, and then immediately after comment about getting the baseline, which makes no sense.

How did you arrive at the numbers of 42 and 26 for the baseline? They smell like magic numbers to me.

review: Needs Fixing (code*)

[Robert Collins (lifeless)]

On Wed, Nov 10, 2010 at 6:17 PM, Steve Kowalik <email address hidden> wrote:
> Review: Needs Fixing code*
> Hi,
>
> With in your test, you look to make a PPA, and then 5 lines later, make a different PPA, but assigned to the same variable, and *then* you make a 3rd PPA assigned to the same variable. This is confusing.

It should be two. Doh. Deleting the spurious one.

> You also set a baseline variable, and then immediately after comment about getting the baseline, which makes no sense.

I'll s/Get/Assess/

> How did you arrive at the numbers of 42 and 26 for the baseline? They smell like magic numbers to me.

They are magic numbers. They are they measured numbers that the page takes.

-Rob

[Stuart Bishop (stub)]

review: Approve

[Stuart Bishop (stub)]

In the name of the senate and peoples of Rome.

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-11-08 12:12:29 +0000
+++ lib/lp/registry/model/distroseries.py	2010-11-10 05:56:09 +0000
@@ -984,16 +984,15 @@
         source_package_ids = [
             package_name.id for package_name in source_package_names]
         releases = SourcePackageRelease.select("""
-            SourcePackageName.id IN %s AND
+            SourcePackageRelease.sourcepackagename IN %s AND
             SourcePackageRelease.id =
                 SourcePackagePublishingHistory.sourcepackagerelease AND
             SourcePackagePublishingHistory.id = (
                 SELECT max(spph.id)
                 FROM SourcePackagePublishingHistory spph,
-                     SourcePackageRelease spr, SourcePackageName spn
+                     SourcePackageRelease spr
                 WHERE
-                    spn.id = SourcePackageName.id AND
-                    spr.sourcepackagename = spn.id AND
+                    spr.sourcepackagename = SourcePackageRelease.sourcepackagename AND
                     spph.sourcepackagerelease = spr.id AND
                     spph.archive IN %s AND
                     spph.status IN %s AND

=== modified file 'lib/lp/soyuz/adapters/archivesourcepublication.py'
--- lib/lp/soyuz/adapters/archivesourcepublication.py	2010-11-07 21:54:38 +0000
+++ lib/lp/soyuz/adapters/archivesourcepublication.py	2010-11-10 05:56:09 +0000
@@ -14,11 +14,14 @@
     'ArchiveSourcePublications',
     ]
 
+from collections import defaultdict
 
 from lazr.delegates import delegates
 from zope.component import getUtility
 
 from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
+from canonical.launchpad.interfaces.lpstorm import IStore
+from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.registry.model.distroseries import DistroSeries
 from lp.soyuz.interfaces.publishing import (
     IPublishingSet,
@@ -53,11 +56,12 @@
     """
     delegates(ISourcePackagePublishingHistory)
 
-    def __init__(self, context, unpublished_builds, builds, changesfile):
+    def __init__(self, context, unpublished_builds, builds, changesfile, status_summary):
         self.context = context
         self._unpublished_builds = unpublished_builds
         self._builds = builds
         self._changesfile = changesfile
+        self._status_summary = status_summary
 
     @property
     def sourcepackagerelease(self):
@@ -84,16 +88,8 @@
 
     def getStatusSummaryForBuilds(self):
         """See `ISourcePackagePublishingHistory`."""
-        # XXX Michael Nelson 2009-05-08 bug=373715. It would be nice if
-        # lazr.delegates passed the delegates 'self' for pass-through
-        # methods, then we wouldn't need to proxy this method call via the
-        # IPublishingSet - instead the delegate would call
-        # ISourcePackagePublishingHistory.getStatusSummaryForBuilds() but
-        # using the delegate as self - might not be possible without
-        # duck-typing.
-        return getUtility(
-            IPublishingSet).getBuildStatusSummaryForSourcePublication(self,
-            self.getBuilds)
+        return self._status_summary
+
 
 class ArchiveSourcePublications:
     """`ArchiveSourcePublication` iterator."""
@@ -162,19 +158,39 @@
             return iter(results)
 
         # Load the extra-information for all source publications.
+        # All of this code would be better on an object representing a set of
+        # publications.
         builds_by_source = self.getBuildsBySource()
         unpublished_builds_by_source = self.getUnpublishedBuildsBySource()
         changesfiles_by_source = self.getChangesFileBySource()
+        # Source package names are used by setNewerDistroSeriesVersions:
+        # batch load the used source package names.
+        spn_ids = set()
+        for spph in self._source_publications:
+            spn_ids.add(spph.sourcepackagerelease.sourcepackagenameID)
+        list(IStore(SourcePackageName).find(SourcePackageName,
+            SourcePackageName.id.is_in(spn_ids)))
         DistroSeries.setNewerDistroSeriesVersions(self._source_publications)
+        # Load all the build status summaries at once.
+        publishing_set = getUtility(IPublishingSet)
+        archive_pub_ids = defaultdict(list)
+        for pub in self._source_publications:
+            archive_pub_ids[pub.archive].append(pub.id)
+        status_summaries = {}
+        for archive, pub_ids in archive_pub_ids.items():
+            status_summaries.update(
+                publishing_set.getBuildStatusSummariesForSourceIdsAndArchive(
+                    pub_ids, archive))
 
         # Build the decorated object with the information we have.
         for pub in self._source_publications:
             builds = builds_by_source.get(pub, [])
             unpublished_builds = unpublished_builds_by_source.get(pub, [])
             changesfile = changesfiles_by_source.get(pub, None)
+            status_summary = status_summaries[pub.id]
             complete_pub = ArchiveSourcePublication(
                 pub, unpublished_builds=unpublished_builds, builds=builds,
-                changesfile=changesfile)
+                changesfile=changesfile, status_summary=status_summary)
             results.append(complete_pub)
 
         return iter(results)

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py	2010-10-16 15:09:47 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py	2010-11-10 05:56:09 +0000
@@ -9,19 +9,28 @@
 __all__ = [
     'TestP3APackages',
     'TestPPAPackages',
-    'test_suite',
     ]
 
+from testtools.matchers import (
+    Equals,
+    LessThan,
+    MatchesAny,
+    )
 from zope.security.interfaces import Unauthorized
 
+from canonical.launchpad.webapp import canonical_url
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.soyuz.browser.archive import ArchiveNavigationMenu
 from lp.testing import (
     login,
     login_person,
+    person_logged_in,
     TestCaseWithFactory,
     )
+from lp.testing.matchers import HasQueryCount
+from lp.testing.sampledata import ADMIN_EMAIL
 from lp.testing.views import create_initialized_view
+from lp.testing._webservice import QueryCollector
 
 
 class TestP3APackages(TestCaseWithFactory):
@@ -113,3 +122,79 @@
     def test_specified_name_filter_returns_none_on_empty_filter(self):
         view = self.getPackagesView('field.name_filter=')
         self.assertIs(None, view.specified_name_filter)
+
+    def test_source_query_counts(self):
+        query_baseline = 42
+        # Assess the baseline.
+        collector = QueryCollector()
+        collector.register()
+        self.addCleanup(collector.unregister)
+        ppa = self.factory.makeArchive()
+        viewer = self.factory.makePerson(password="test")
+        browser = self.getUserBrowser(user=viewer)
+        with person_logged_in(viewer):
+            # The baseline has one package, because otherwise the short-circuit
+            # prevents the packages iteration happening at all and we're not
+            # actually measuring scaling appropriately.
+            self.factory.makeSourcePackagePublishingHistory(archive=ppa)
+            url = canonical_url(ppa) + "/+packages"
+        browser.open(url)
+        self.assertThat(collector, HasQueryCount(LessThan(query_baseline)))
+        expected_count = collector.count
+        # We scale with 1 query per distro series because of
+        # getCurrentSourceReleases.
+        expected_count += 1
+        # We need a fuzz of one because if the test is the first to run a 
+        # credentials lookup is done as well (and accrued to the collector).
+        expected_count += 1
+        # Use all new objects - avoids caching issues invalidating the gathered
+        # metrics.
+        login(ADMIN_EMAIL)
+        ppa = self.factory.makeArchive()
+        viewer = self.factory.makePerson(password="test")
+        browser = self.getUserBrowser(user=viewer)
+        with person_logged_in(viewer):
+            for i in range(2):
+                pkg = self.factory.makeSourcePackagePublishingHistory(
+                    archive=ppa)
+                self.factory.makeSourcePackagePublishingHistory(archive=ppa,
+                    distroseries=pkg.distroseries)
+            url = canonical_url(ppa) + "/+packages"
+        browser.open(url)
+        self.assertThat(collector, HasQueryCount(LessThan(expected_count)))
+
+    def test_binary_query_counts(self):
+        query_baseline = 26
+        # Assess the baseline.
+        collector = QueryCollector()
+        collector.register()
+        self.addCleanup(collector.unregister)
+        ppa = self.factory.makeArchive()
+        viewer = self.factory.makePerson(password="test")
+        browser = self.getUserBrowser(user=viewer)
+        with person_logged_in(viewer):
+            # The baseline has one package, because otherwise the short-circuit
+            # prevents the packages iteration happening at all and we're not
+            # actually measuring scaling appropriately.
+            self.factory.makeBinaryPackagePublishingHistory(archive=ppa)
+            url = canonical_url(ppa) + "/+packages"
+        browser.open(url)
+        self.assertThat(collector, HasQueryCount(
+            MatchesAny(LessThan(query_baseline), Equals(query_baseline))))
+        expected_count = collector.count
+        # Use all new objects - avoids caching issues invalidating the gathered
+        # metrics.
+        login(ADMIN_EMAIL)
+        ppa = self.factory.makeArchive()
+        viewer = self.factory.makePerson(password="test")
+        browser = self.getUserBrowser(user=viewer)
+        with person_logged_in(viewer):
+            for i in range(2):
+                pkg = self.factory.makeBinaryPackagePublishingHistory(
+                    archive=ppa)
+                self.factory.makeBinaryPackagePublishingHistory(archive=ppa,
+                    distroarchseries=pkg.distroarchseries)
+            url = canonical_url(ppa) + "/+packages"
+        browser.open(url)
+        self.assertThat(collector, HasQueryCount(
+            MatchesAny(Equals(expected_count), LessThan(expected_count))))

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2010-11-07 22:51:53 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2010-11-10 05:56:09 +0000
@@ -1079,8 +1079,7 @@
             `IBinaryPackagePublishingHistory`.
         """
 
-    def getBuildStatusSummariesForSourceIdsAndArchive(source_ids, archive,
-        get_builds=None):
+    def getBuildStatusSummariesForSourceIdsAndArchive(source_ids, archive):
         """Return a summary of the build statuses for source publishing ids.
 
         This method collects all the builds for the provided source package
@@ -1095,9 +1094,6 @@
         :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:
                 {
@@ -1108,8 +1104,7 @@
         :rtype: ``dict``.
         """
 
-    def getBuildStatusSummaryForSourcePublication(source_publication,
-        get_builds=None):
+    def getBuildStatusSummaryForSourcePublication(source_publication):
         """Return a summary of the build statuses for this source
         publication.
 
@@ -1117,13 +1112,6 @@
         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/interfaces/sourcepackagerelease.py'
--- lib/lp/soyuz/interfaces/sourcepackagerelease.py	2010-08-25 00:29:36 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerelease.py	2010-11-10 05:56:09 +0000
@@ -98,6 +98,7 @@
     files = Attribute("IBinaryPackageFile entries for this "
         "sourcepackagerelease")
     sourcepackagename = Attribute("SourcePackageName table reference")
+    sourcepackagenameID = Attribute("SourcePackageName id.")
     upload_distroseries = Attribute("The distroseries in which this package "
         "was first uploaded in Launchpad")
     publishings = Attribute("MultipleJoin on SourcepackagePublishing")

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-11-09 00:01:48 +0000
+++ lib/lp/soyuz/model/publishing.py	2010-11-10 05:56:09 +0000
@@ -1699,41 +1699,32 @@
         return result_set.one()
 
     def getBuildStatusSummariesForSourceIdsAndArchive(self, source_ids,
-        archive, get_builds=None):
+        archive):
         """See `IPublishingSet`."""
         # source_ids can be None or an empty sequence.
         if not source_ids:
             return {}
 
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        if len(source_ids) == 1 and get_builds is not None:
-            # Fake out the DB access for ArchivePublication
-            source_pubs = list(store.find(
+        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(source_ids),
+                SourcePackagePublishingHistory.id.is_in(
+                    pubs_without_builds),
                 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)
@@ -1774,8 +1765,7 @@
 
         return source_build_statuses
 
-    def getBuildStatusSummaryForSourcePublication(self, source_publication,
-        get_builds=None):
+    def getBuildStatusSummaryForSourcePublication(self, source_publication):
         """See `ISourcePackagePublishingHistory`.getStatusSummaryForBuilds.
 
         This is provided here so it can be used by both the SPPH as well
@@ -1785,7 +1775,7 @@
         """
         source_id = source_publication.id
         return self.getBuildStatusSummariesForSourceIdsAndArchive([source_id],
-            source_publication.archive, get_builds=get_builds)[source_id]
+            source_publication.archive)[source_id]
 
     def requestDeletion(self, sources, removed_by, removal_comment=None):
         """See `IPublishingSet`."""