On Mar 30, 2011, at 5:14 AM, Andrew Bennetts wrote:
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.
Agreed. I've seen this behavior crop up shocking close to the top of profiles for Calendar Server as well, so I'm very glad to hear you've undertaken this work. The current default is almost certainly wrong; the only code which has practically ever used it was the HTML traceback stuff, which most serious users disable for security reasons anyway. Debugging is kind of a process-global thing, most of the time. I think maybe we should have 'twisted.python.debug' which is the main thing that all these features use, and then a setDebugging for each system (Failure, Deferred, reactor; and ideally, eventually stuff like web, mail, conch too) to turn on these expensive-but-occasionally-worthwhile features. But I'd be happy if this change did _nothing_ but make Failure simply default to not capturing globals and locals, and add a flag to explicitly request this behavior. Like you say: the information is always captured but almost never used. -glyph