On 10 August 2016 at 04:43, Giampaolo Rodola' <g.rod...@gmail.com> wrote:
> > > On Tue, Aug 9, 2016 at 3:30 PM, Nick Coghlan <ncogh...@gmail.com> wrote: > >> On 9 August 2016 at 23:26, Nick Coghlan <ncogh...@gmail.com> wrote: >> >>> On 9 August 2016 at 06:18, Guido van Rossum <gu...@python.org> wrote: >>> >>>> I think Nick would be interested in understanding why this is the case. >>>> What does the decorator do that could be so expensive? >>>> >>> >>> Reviewing https://hg.python.org/cpython/file/default/Lib/contextlib.py >>> #l57, Chris's analysis seems plausible to me >>> >> >> Sorry Wolfgang - I missed that Chris was expanding on a comparison you >> initially made! >> >> Either way, I agree that aspect does make up the bulk of the difference >> in speed, so moving to C likely wouldn't help much. However, the speed >> difference relative to the simpler warppers is far less defensible - I >> think there are some opportunities for improvement there, especially around >> moving introspection work out of _GeneratorContextManager.__init__ and >> into the contextmanager decorator itself. >> > > By moving the __doc__ introspection out of __init__ and by introducing > __slots__ I got from -4.37x to -3.16x (__slot__ speedup was about +0.3x). > The draft changes aren't preserving the semantics (which may suggest we have some missing test cases), so we can't take timing numbers just yet. > Chris' SimplerContextManager solution is faster because it avoids the > factory function but that is necessary for supporting the decoration of > methods. Here's a PoC: > > > diff --git a/Lib/contextlib.py b/Lib/contextlib.py > index 7d94a57..45270dd 100644 > --- a/Lib/contextlib.py > +++ b/Lib/contextlib.py > @@ -2,7 +2,7 @@ > import abc > import sys > from collections import deque > -from functools import wraps > +from functools import wraps, update_wrapper > > __all__ = ["contextmanager", "closing", "AbstractContextManager", > "ContextDecorator", "ExitStack", "redirect_stdout", > @@ -57,25 +57,18 @@ class ContextDecorator(object): > class _GeneratorContextManager(ContextDecorator, AbstractContextManager): > """Helper for @contextmanager decorator.""" > > + __slots__ = ['gen', 'funcak'] > + > Note that this is is still keeping the __dict__ allocation (you'd have to make ContexDecorator and AbstractContextManager use __slots__ as well to eliminate it). While there's likely some speedup just from the dict->descriptor shift, let's see how far we can improve *without* taking the __slots__ step, and then consider the question of adopting __slots__ (and the backwards compatibility implications if we take it as far as eliminating __dict__) separately. > def __init__(self, func, args, kwds): > self.gen = func(*args, **kwds) > - self.func, self.args, self.kwds = func, args, kwds > - # Issue 19330: ensure context manager instances have good > docstrings > - doc = getattr(func, "__doc__", None) > - if doc is None: > - doc = type(self).__doc__ > - self.__doc__ = doc > - # Unfortunately, this still doesn't provide good help output when > - # inspecting the created context manager instances, since pydoc > - # currently bypasses the instance docstring and shows the > docstring > - # for the class instead. > - # See http://bugs.python.org/issue19404 for more details. > + self.funcak = func, args, kwds > This actually gives me an idea - we should compare the performance of the current code with that of using a functools.partial object defined in the decorator function, so only the partial object needs to be passed in to _GeneratorContextManager, and we can drop the args and kwds parameters entirely. That should move some more of the work out of the per-use code in _GeneratorContextManager.__init__ and into the per-definition code in the contextmanager decorator, and "self.gen = func()" should then be faster than "self.gen = func(*args, **kwds)". > def _recreate_cm(self): > # _GCM instances are one-shot context managers, so the > # CM must be recreated each time a decorated function is > # called > - return self.__class__(self.func, self.args, self.kwds) > + func, args, kwds = self.funcak > + return self.__class__(func, args, kwds) > > def __enter__(self): > try: > @@ -157,6 +150,8 @@ def contextmanager(func): > @wraps(func) > def helper(*args, **kwds): > return _GeneratorContextManager(func, args, kwds) > + > + update_wrapper(helper, func) > return helper > As Chris noted, this probably isn't having the effect you want - to move the introspection code out to the decorator, you still need to pull __doc__ from the function (at decoration time) and set it on the _GeneratorContextManager instance (at instance creation time). However, we're getting down to discussing proposed patch details now, so it's probably time to file an enhancement issue on the tracker and move further discussion there :) Cheers, Nick. P.S. I head down to Melbourne for PyCon Australia tomorrow, so I probably won't be too responsive until the conference sprints start on Monday. -- Nick Coghlan | ncogh...@gmail.com | Brisbane, Australia
_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
