Merge lp:~ev/apport/973494 into lp:apport

[Evan]

We've found apport-retrace and the code that wraps it in the crash database to be a bottleneck (bug 973494). In trying to improve the speed in which we can retrace core files (which is currently about 3/min), I noticed that we're constructing a new apt.Cache twice with the same data and calling update() on it twice.

This branch fixes that, and provides the following speed improvement:

ubuntu@server-8149:~$ ./judge/judge 'apport-retrace _usr_bin_gedit.1000.crash -o new.crash -S retracer/config -C cache
-c' './with_changes'
v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^v^

    n mean sd min max cmd
       50 29566.9ms 1206.3 27047.6 36682.3 apport-retrace _usr_bin_gedit.1000.crash -o new.crash -S retracer/config -C cache -c
          50 23592.5ms 648.8 21846.0 26651.0 ./with_changes
          -5974.360ms -20.2% p=0.000
          difference is significant at 95.0% confidence (p=0.000):
          based on these samples, suggested sample size is n>=5 to have a 1493.59ms confidence interval

I dropped the workaround for setting apt.Cache(rootdir='/') at the end of install_packages() in favor of explicitly calling apt.Cache with that parameter in _cache().

[Evan (ev)]

I should note that this doesn't need to go into 12.04. If you're happy with the change, I can build this out of a PPA.

[Martin Pitt (pitti)]

+ self._sandbox_apt_cache = None
+ if self._sandbox_apt_cache or not self._apt_cache:

This looks a bit weird. I presume it is just a copy&paste error, I'll fix that, and also the documentation for the new _sandbox_cache. Thanks!

review: Approve

=== modified file 'backends/packaging-apt-dpkg.py'
--- backends/packaging-apt-dpkg.py	2012-03-22 17:10:58 +0000
+++ backends/packaging-apt-dpkg.py	2012-04-10 10:31:20 +0000
@@ -28,6 +28,7 @@
 
     def __init__(self):
         self._apt_cache = None
+        self._sandbox_apt_cache = None
         self._contents_dir = None
         self._mirror = None
 
@@ -44,15 +45,36 @@
     def _cache(self):
         '''Return apt.Cache() (initialized lazily).'''
 
-        if not self._apt_cache:
+        self._sandbox_apt_cache = None
+        if self._sandbox_apt_cache or not self._apt_cache:
             try:
                 # avoid spewage on stdout
-                self._apt_cache = apt.Cache(apt.progress.base.OpProgress())
+                progress = apt.progress.base.OpProgress()
+                self._apt_cache = apt.Cache(progress, rootdir='/')
             except AttributeError:
                 # older python-apt versions do not yet have above argument
-                self._apt_cache = apt.Cache()
+                self._apt_cache = apt.Cache(rootdir='/')
         return self._apt_cache
 
+    def _sandbox_cache(self, aptroot, apt_sources, fetchProgress):
+        '''Return apt.Cache(rootdir=) (initialized lazily).
+           Clear the package selection on subsequent calls.'''
+
+        self._apt_cache = None
+        if not self._sandbox_apt_cache:
+            self._build_apt_sandbox(aptroot, apt_sources)
+            rootdir = os.path.abspath(aptroot)
+            self._sandbox_apt_cache = apt.Cache(rootdir=rootdir)
+            try:
+                # We don't need to update this multiple times.
+                self._sandbox_apt_cache.update(fetchProgress)
+            except apt.cache.FetchFailedException as e:
+                raise SystemError(str(e))
+            self._sandbox_apt_cache.open()
+        else:
+            self._sandbox_apt_cache.clear()
+        return self._sandbox_apt_cache
+
     def _apt_pkg(self, package):
         '''Return apt.Cache()[package] (initialized lazily).
 
@@ -491,18 +513,20 @@
             tmp_aptroot = True
             aptroot = tempfile.mkdtemp()
 
-        self._build_apt_sandbox(aptroot, apt_sources)
-
         if verbose:
             fetchProgress = apt.progress.text.AcquireProgress()
         else:
             fetchProgress = apt.progress.base.AcquireProgress()
-        c = apt.Cache(rootdir=os.path.abspath(aptroot))
-        try:
-            c.update(fetchProgress)
-        except apt.cache.FetchFailedException as e:
-            raise SystemError(str(e))
-        c.open()
+        if not tmp_aptroot:
+            c = self._sandbox_cache(aptroot, apt_sources, fetchProgress)
+        else:
+            self._build_apt_sandbox(aptroot, apt_sources)
+            c = apt.Cache(rootdir=os.path.abspath(aptroot))
+            try:
+                c.update(fetchProgress)
+            except apt.cache.FetchFailedException as e:
+                raise SystemError(str(e))
+            c.open()
 
         obsolete = ''
 
@@ -558,12 +582,6 @@
         assert not real_pkgs, 'apt fetcher did not fetch these packages: ' \
             + ' '.join(real_pkgs)
 
-        # work around python-apt bug that causes parts of the Cache(rootdir=)
-        # argument configuration to be persistent; this resets the apt
-        # configuration to system defaults again
-        apt.Cache(rootdir='/')
-        self._apt_cache = None
-
         return obsolete
 
     def package_name_glob(self, nameglob):