[Python-Dev] Rewrite @contextlib.contextmanager in C

[Nick Coghlan]

On 10 August 2016 at 04:43, Giampaolo Rodola' <g.rodola at gmail.com> wrote:

>
>
> On Tue, Aug 9, 2016 at 3:30 PM, Nick Coghlan <ncoghlan at gmail.com> wrote:
>
>> On 9 August 2016 at 23:26, Nick Coghlan <ncoghlan at gmail.com> wrote:
>>
>>> On 9 August 2016 at 06:18, Guido van Rossum <guido at 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   |   ncoghlan at gmail.com   |   Brisbane, Australia
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-dev/attachments/20160811/fc242a85/attachment.html>