launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-799785 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-799785/+merge/65373

This branch fixes bug 799785: Timeout when trying to set the translation focus for Ubuntu 

The OOPS report from this bug shows one very long query, which is issued in Snapshot(distribution), called in lp.app.browser.launchpadform.LaunchpadEditFormView.updateContextFromData(), when Distribution.has_published_sources is evaluated.

The query is a UNION of two subqueries; an EXPLAIN ANALYZE on staging (thanks Deryck!) of it showed:

- one of the sub-queries needed 14 seconds; the other sub-query needed <20 milliseconds. 
- the first sub-query needed 10 seconds (or even more -- I did not look that carefully at every line of the EXPLAIN ANALYZE output..,) to sort the result.
- Merging and sorting the two sub-queries required an external disk merge, occupying 46912kB; it took 7 seconds.

The property Distribution.has_published_sources tells us, if _any_ published source exists, so we really do not need all the sorting and merging of sub-queries.

Another run of EXPLAIN ANALZYE of only the first, originally slow, query, but now originating from

   archive.getPublishedSources().order_by().is_empty()

needed < 40 milliseoncds.

So we can simply loop over self.all_distro_archives, and leave the loop early for the first non-empty result set.

Since we have just two archive_sources even for Ubuntu, I am not concerned about a slow loop execution, caused by a loop over a large number of empty archive_sources for any distribution.

I did not add any test (not sure if we can properly test SQL performance...); the existing short test in lib/lp/registry/doc/distribution.txt still passes.

So, just one existing check:

./bin/test registry -vvt doc.distribution.txt

no lint
-- 
https://code.launchpad.net/~adeuring/launchpad/bug-799785/+merge/65373
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-799785 into lp:launchpad.

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2011-06-17 19:47:36 +0000
+++ lib/lp/registry/model/distribution.py	2011-06-21 16:34:38 +0000
@@ -30,7 +30,6 @@
     SQL,
     )
 from storm.store import (
-    EmptyResultSet,
     Store,
     )
 from zope.component import getUtility
@@ -1850,11 +1849,10 @@
 
     @property
     def has_published_sources(self):
-        archives_sources = EmptyResultSet()
         for archive in self.all_distro_archives:
-            archives_sources = archives_sources.union(
-                archive.getPublishedSources())
-        return not archives_sources.is_empty()
+            if not archive.getPublishedSources().order_by().is_empty():
+                return True
+        return False
 
 
 class DistributionSet: