John Arbash Meinel wrote: […]
I think walking the frames and copying the dicts is also expensive. That is what the bug you linked to me was about. (First, walking everything and using __dict__.copy() was a bit expensive, and second that the safe_repr() calls were turning 1GB strings into a new 1GB+ string.)
Well, if we take my proposal to not (by default at least) capture the frames in the first place this isn't an issue is it? The only other copy done in Failure is the copy of self.__dict__ in __getstate__, which is just a shallow copy of one dict, so fairly cheap.
The one other step that I think we need, is that 'maybeDeferred' also always traps into a Failure object, and we'd want that to check Deferred.debug first.
Good point!
I do wonder if Failure should just be checking Deferred.debug before automatically including a traceback. I'm not really sure about logical layering of twisted modules, though. Certainly 'twisted.python.failure' seems a lower layer than 'twisted.internet.defer'.
Yes, checking Deferred.debug in twisted.python.failure would be bad layering. twisted.internet.defer only invokes Failure in a couple of places, so it's not so onerous to make sure it invokes it to avoid capturing tracebacks unless it means to. That said, it might be a good idea to change Failure to *not* capture tracebacks by default when invoked as Failure(). Perhaps add a setDebugging toggle to twisted.python.failure too. If an explicit traceback is passed to the constructor it would still be captured, and perhaps provide an alternative constructor for the current “capture everything” behaviour (perhaps via a new optional flag for __init__, perhaps via a new name DetailedFailure(), perhaps via a classmethod except failure is not a new-style class and can't be one without changing to subclass Exception…) for the rare cases when people want that. It's a pity you can't just use Failure(*sys.exc_info()) because the parameters are in the wrong order. Certainly the current behaviour of doing a costly capturing of traceback and frame contents seems like the wrong default, given how rarely I've seen anyone use Failure.printDetailedTraceback. -Andrew.