launchpad-reviewers team mailing list archive

[Andrew Bennetts <andrew.bennetts@xxxxxxxxxxxxx>]

Andrew Bennetts has proposed merging lp:~spiv/launchpad/codehosting-cheaper-failures into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~spiv/launchpad/codehosting-cheaper-failures/+merge/55736

This branch is a workaround for <http://twistedmatrix.com/trac/ticket/5011>.  jam reports that it roughly halves the time it takes for codehosting to service a simple bzr request.

The basic problem is that Twisted is currently astonishingly slow at handling exceptions raised in Deferred callbacks (and errbacks), because it captures and then stringifies every stack frame, including all globals and locals.  This seems to be especially true in our situation where we have deep call stacks, and are wrapping synchronous APIs in an async interface.  I go into the details on the Twisted bug report if you are curious.

So to workaround this issue, this patch intercepts most places in the codehosting vfs modules that raise errors in callbacks and instead explicitly returns Failures with no tracebacks (just an exception value).

I have a patch for Twisted that is likely to be accepted soon, but until we have a fixed version of Twisted deployed with Launchpad this workaround is probably worthwhile.
-- 
https://code.launchpad.net/~spiv/launchpad/codehosting-cheaper-failures/+merge/55736
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~spiv/launchpad/codehosting-cheaper-failures into lp:launchpad.

=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2010-12-12 23:09:36 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2011-03-31 11:32:37 +0000
@@ -116,6 +116,7 @@
     get_readonly_transport,
     TranslationError,
     )
+from lp.services.twistedsupport import no_traceback_failures
 from lp.services.twistedsupport.xmlrpc import (
     DeferredBlockingProxy,
     trap_fault,
@@ -399,17 +400,18 @@
         deferred = self._branchfs_client.translatePath(
             '/' + virtual_url_fragment)
 
-        def path_not_translated(failure):
+        def path_not_translated(fail):
             trap_fault(
-                failure, faults.PathTranslationError, faults.PermissionDenied)
-            raise NoSuchFile(virtual_url_fragment)
+                fail, faults.PathTranslationError, faults.PermissionDenied)
+            return failure.Failure(NoSuchFile(virtual_url_fragment))
 
-        def unknown_transport_type(failure):
-            failure.trap(UnknownTransportType)
-            raise NoSuchFile(virtual_url_fragment)
+        def unknown_transport_type(fail):
+            fail.trap(UnknownTransportType)
+            return failure.Failure(NoSuchFile(virtual_url_fragment))
 
         deferred.addCallbacks(
-            self._transport_dispatch.makeTransport, path_not_translated)
+            no_traceback_failures(self._transport_dispatch.makeTransport),
+            path_not_translated)
         deferred.addErrback(unknown_transport_type)
         return deferred
 
@@ -480,6 +482,7 @@
             getUtility(IBranchLookup).getIdAndTrailingPath(
                 virtual_url_fragment))
 
+        @no_traceback_failures
         def process_result((branch_id, trailing)):
             if branch_id is None:
                 raise NoSuchFile(virtual_url_fragment)
@@ -511,11 +514,13 @@
         deferred = AsyncVirtualTransport._getUnderylingTransportAndPath(
             self, relpath)
 
+        @no_traceback_failures
         def maybe_make_branch_in_db(failure):
             # Looks like we are trying to make a branch.
             failure.trap(NoSuchFile)
             return self.server.createBranch(self._abspath(relpath))
 
+        @no_traceback_failures
         def real_mkdir((transport, path)):
             return getattr(transport, 'mkdir')(path, mode)
 
@@ -532,8 +537,9 @@
         else:
             deferred = defer.succeed(None)
         deferred = deferred.addCallback(
-            lambda ignored: AsyncVirtualTransport.rename(
-                self, rel_from, rel_to))
+            no_traceback_failures(
+                lambda ignored: AsyncVirtualTransport.rename(
+                    self, rel_from, rel_to)))
         return deferred
 
     def rmdir(self, relpath):
@@ -607,7 +613,7 @@
         """
         deferred = self._branchfs_client.createBranch(virtual_url_fragment)
 
-        def translate_fault(failure):
+        def translate_fault(fail):
             # We turn faults.NotFound into a PermissionDenied, even
             # though one might think that it would make sense to raise
             # NoSuchFile. Sadly, raising that makes the client do "clever"
@@ -616,11 +622,12 @@
             # exist. You may supply --create-prefix to create all leading
             # parent directories", which is just misleading.
             fault = trap_fault(
-                failure, faults.NotFound, faults.PermissionDenied)
+                fail, faults.NotFound, faults.PermissionDenied)
             faultString = fault.faultString
             if isinstance(faultString, unicode):
                 faultString = faultString.encode('utf-8')
-            raise PermissionDenied(virtual_url_fragment, faultString)
+            return failure.Failure(
+                PermissionDenied(virtual_url_fragment, faultString))
 
         return deferred.addErrback(translate_fault)
 
@@ -671,6 +678,7 @@
         deferred = self._branchfs_client.translatePath(
             '/' + virtual_url_fragment)
 
+        @no_traceback_failures
         def got_path_info((transport_type, data, trailing_path)):
             if transport_type != BRANCH_TRANSPORT:
                 raise NotABranchPath(virtual_url_fragment)
@@ -699,6 +707,7 @@
                 data['id'], stacked_on_url, last_revision,
                 control_string, branch_string, repository_string)
 
+        @no_traceback_failures
         def handle_error(failure=None, **kw):
             # It gets really confusing if we raise an exception from this
             # method (the branch remains locked, but this isn't obvious to

=== modified file 'lib/lp/codehosting/vfs/branchfsclient.py'
--- lib/lp/codehosting/vfs/branchfsclient.py	2010-09-22 18:32:22 +0000
+++ lib/lp/codehosting/vfs/branchfsclient.py	2011-03-31 11:32:37 +0000
@@ -17,6 +17,7 @@
 from twisted.internet import defer
 
 from lp.code.interfaces.codehosting import BRANCH_TRANSPORT
+from lp.services.twistedsupport import no_traceback_failures
 
 
 class NotInCache(Exception):
@@ -128,5 +129,5 @@
         except NotInCache:
             deferred = self._codehosting_endpoint.callRemote(
                 'translatePath', self._user_id, path)
-            deferred.addCallback(self._addToCache, path)
+            deferred.addCallback(no_traceback_failures(self._addToCache), path)
             return deferred

=== modified file 'lib/lp/codehosting/vfs/transport.py'
--- lib/lp/codehosting/vfs/transport.py	2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/vfs/transport.py	2011-03-31 11:32:37 +0000
@@ -37,10 +37,12 @@
     unregister_transport,
     )
 from twisted.internet import defer
+from twisted.python.failure import Failure
 
 from lp.services.twistedsupport import (
     extract_result,
     gatherResults,
+    no_traceback_failures,
     )
 
 
@@ -119,7 +121,7 @@
         :param failure: A `twisted.python.failure.Failure`.
         """
         failure.trap(TranslationError)
-        raise NoSuchFile(failure.value.virtual_url_fragment)
+        return Failure(NoSuchFile(failure.value.virtual_url_fragment))
 
     def _call(self, method_name, relpath, *args, **kwargs):
         """Call a method on the backing transport, translating relative,
@@ -131,7 +133,14 @@
         """
         def call_method((transport, path)):
             method = getattr(transport, method_name)
-            return method(path, *args, **kwargs)
+            try:
+                return method(path, *args, **kwargs)
+            except BaseException, e:
+                # It's much cheaper to explicitly construct a Failure than to
+                # let Deferred build automatically, because the automatic one
+                # will capture the traceback and perform an expensive
+                # stringification on it.
+                return Failure(e)
 
         deferred = self._getUnderylingTransportAndPath(relpath)
         deferred.addCallback(call_method)
@@ -169,6 +178,7 @@
 
     def iter_files_recursive(self):
         deferred = self._getUnderylingTransportAndPath('.')
+        @no_traceback_failures
         def iter_files((transport, path)):
             return transport.clone(path).iter_files_recursive()
         deferred.addCallback(iter_files)
@@ -176,6 +186,7 @@
 
     def listable(self):
         deferred = self._getUnderylingTransportAndPath('.')
+        @no_traceback_failures
         def listable((transport, path)):
             return transport.listable()
         deferred.addCallback(listable)
@@ -219,11 +230,12 @@
         from_deferred = self._getUnderylingTransportAndPath(rel_from)
         deferred = gatherResults([to_deferred, from_deferred])
 
+        @no_traceback_failures
         def check_transports_and_rename(
             ((to_transport, to_path), (from_transport, from_path))):
             if to_transport.base != from_transport.base:
-                raise TransportNotPossible(
-                    'cannot move between underlying transports')
+                return Failure(TransportNotPossible(
+                    'cannot move between underlying transports'))
             return getattr(from_transport, 'rename')(from_path, to_path)
 
         deferred.addCallback(check_transports_and_rename)

=== modified file 'lib/lp/services/twistedsupport/__init__.py'
--- lib/lp/services/twistedsupport/__init__.py	2010-10-15 15:01:39 +0000
+++ lib/lp/services/twistedsupport/__init__.py	2011-03-31 11:32:37 +0000
@@ -13,6 +13,7 @@
     ]
 
 
+import functools
 import StringIO
 import sys
 
@@ -21,16 +22,17 @@
     threads,
     reactor as default_reactor,
     )
-from twisted.python.util import mergeFunctionMetadata
+from twisted.python.failure import Failure
 
 
 def defer_to_thread(function):
     """Run in a thread and return a Deferred that fires when done."""
 
+    @functools.wraps(function)
     def decorated(*args, **kwargs):
         return threads.deferToThread(function, *args, **kwargs)
 
-    return mergeFunctionMetadata(function, decorated)
+    return decorated
 
 
 def gatherResults(deferredList):
@@ -63,6 +65,7 @@
         sys.stderr = stream
         return result
 
+    @functools.wraps(function)
     def wrapper(*arguments, **keyword_arguments):
         saved_stderr = sys.stderr
         ignored_stream = StringIO.StringIO()
@@ -70,7 +73,7 @@
         d = defer.maybeDeferred(function, *arguments, **keyword_arguments)
         return d.addBoth(set_stderr, saved_stderr)
 
-    return mergeFunctionMetadata(function, wrapper)
+    return wrapper
 
 
 def extract_result(deferred):
@@ -114,3 +117,19 @@
         return passthrough
     return d.addBoth(cancel_timeout)
 
+
+def no_traceback_failures(func):
+    """Decorator to return traceback-less Failures instead of raising errors.
+    
+    This is useful for functions used as callbacks or errbacks for a Deferred.
+    Traceback-less failures are much faster than the automatic Failures
+    Deferred constructs internally.
+    """
+    @functools.wraps(func)
+    def wrapped(*args, **kwargs):
+        try:
+            return func(*args, **kwargs)
+        except BaseException, e:
+            return Failure(e)
+
+    return wrapped