launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-816870 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-816870/+merge/108728

This branch hopefully fixes the parts of Distribution:+search that contribute most to its almost unending timeouts.

Firstly, has_exact_matches no longer does a slow count(). is_empty() is fine for its purposes. Secondly, searchBinaryPackages has its extremely hideous and slow 8 table join replaced with a slightly less hideous but 50x faster set of LIKEs.

DistributionSourcePackageCache.binpkgnames is a space-separated string of all the binaries produced by the source. We'd ideally replace it with something like an array of SPN IDs, but schema changes like that aren't quite cheap yet, so I achieve an exact match by forcing there to be a word boundary on both sides of the name. A regular expression would be a lot prettier, but it ended up being 300ms vs 90ms, so I decided the LIKEs won for now.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-816870/+merge/108728
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-816870 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distribution.py'
--- lib/lp/registry/browser/distribution.py	2012-05-18 07:51:18 +0000
+++ lib/lp/registry/browser/distribution.py	2012-06-05 12:36:21 +0000
@@ -585,7 +585,7 @@
 
     @property
     def has_exact_matches(self):
-        return self.exact_matches.count() > 0
+        return not self.exact_matches.is_empty()
 
     @property
     def has_matches(self):

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2012-05-14 13:32:03 +0000
+++ lib/lp/registry/model/distribution.py	2012-06-05 12:36:21 +0000
@@ -179,7 +179,6 @@
 from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
 from lp.soyuz.interfaces.publishing import active_publishing_status
 from lp.soyuz.model.archive import Archive
-from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
 from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
 from lp.soyuz.model.distributionsourcepackagerelease import (
@@ -1167,52 +1166,39 @@
         # results will only see DSPs
         return DecoratedResultSet(dsp_caches_with_ranks, result_to_dsp)
 
-    @property
-    def _binaryPackageSearchClause(self):
-        """Return a Storm match clause for binary package searches."""
-        # This matches all DistributionSourcePackageCache rows that have
-        # a source package that generated the BinaryPackageName that
-        # we're searching for.
+    def searchBinaryPackages(self, package_name, exact_match=False):
+        """See `IDistribution`."""
         from lp.soyuz.model.distributionsourcepackagecache import (
             DistributionSourcePackageCache,
             )
-        return (
-            DistroSeries.distribution == self,
-            DistroSeries.status != SeriesStatus.OBSOLETE,
-            DistroArchSeries.distroseries == DistroSeries.id,
-            BinaryPackageBuild.distro_arch_series == DistroArchSeries.id,
-            BinaryPackageRelease.build == BinaryPackageBuild.id,
-            (BinaryPackageBuild.source_package_release ==
-                SourcePackageRelease.id),
-            SourcePackageRelease.sourcepackagename == SourcePackageName.id,
-            DistributionSourcePackageCache.sourcepackagename ==
-                SourcePackageName.id,
+        store = Store.of(self)
+
+        select_spec = (DistributionSourcePackageCache,)
+
+        find_spec = (
+            DistributionSourcePackageCache.distribution == self,
             DistributionSourcePackageCache.archiveID.is_in(
                 self.all_distro_archive_ids))
 
-    def searchBinaryPackages(self, package_name, exact_match=False):
-        """See `IDistribution`."""
-        from lp.soyuz.model.distributionsourcepackagecache import (
-            DistributionSourcePackageCache,
-            )
-        store = Store.of(self)
-
-        select_spec = (DistributionSourcePackageCache,)
-
         if exact_match:
-            find_spec = self._binaryPackageSearchClause + (
-                BinaryPackageRelease.binarypackagename
-                    == BinaryPackageName.id,
-                )
-            match_clause = (BinaryPackageName.name == package_name,)
+            # To match BinaryPackageName.name exactly requires a very
+            # slow 8 table join. So let's instead use binpkgnames, with
+            # an ugly set of LIKEs matching spaces or either end of the
+            # string on either side of the name. A regex is several
+            # times slower and harder to escape.
+            match_clause = (Or(
+                DistributionSourcePackageCache.binpkgnames.like(
+                    '%% %s %%' % package_name.lower()),
+                DistributionSourcePackageCache.binpkgnames.like(
+                    '%% %s' % package_name.lower()),
+                DistributionSourcePackageCache.binpkgnames.like(
+                    '%s %%' % package_name.lower()),
+                DistributionSourcePackageCache.binpkgnames ==
+                    package_name.lower()), )
         else:
             # In this case we can use a simplified find-spec as the
             # binary package names are present on the
             # DistributionSourcePackageCache records.
-            find_spec = (
-                DistributionSourcePackageCache.distribution == self,
-                DistributionSourcePackageCache.archiveID.is_in(
-                    self.all_distro_archive_ids))
             match_clause = (
                 DistributionSourcePackageCache.binpkgnames.like(
                     "%%%s%%" % package_name.lower()),)

=== modified file 'lib/lp/soyuz/browser/tests/distribution-views.txt'
--- lib/lp/soyuz/browser/tests/distribution-views.txt	2011-05-16 11:20:41 +0000
+++ lib/lp/soyuz/browser/tests/distribution-views.txt	2012-06-05 12:36:21 +0000
@@ -195,9 +195,9 @@
     >>> distro_pkg_search_view.distroseries_names
     {u'mozilla-firefox': u'warty'}
     >>> distro_pkg_search_view = create_initialized_view(
-    ...     ubuntu, name="+search", form={'text': 'foobar'})
+    ...     ubuntu, name="+search", form={'text': 'mozilla-firefox'})
     >>> distro_pkg_search_view.distroseries_names
-    {u'foobar': ''}
+    {u'mozilla-firefox': u'warty'}
 
 Another helper on the DistributionPackageSearchView is the
 matching_binary_names property which can be used by templates to get