faster-cpython / ideas

1.67k stars 49 forks source link

single-arg __exit__ function #550

Closed iritkatriel closed 1 year ago

iritkatriel commented 1 year ago

I went over the __exit__ functions in the stdlib and the vast majority take *args as input and do nothing with it. So every time an __exit__ is called, the interpreter either pushes 3 Nones to the stack and calls a 3-arg method, or (if there is an exception) constructs the (exc, val tb) from the exception, pushes the 3 values to the stack and then calls the 3-arg method. If __exit__ took just the exception object, the interpreter would not need to construct these tuples, and it would just call a single-arg function, which is faster.

Microbenchmarks showed this makes entering and exiting a context manager about 13% faster when no exception is raised.

If this was just about performance I probably wouldn't be that bothered. But the current __exit__ API is a wart in the language that confuses programmers, particularly beginners. As of Python 3.11 it is completely redundant, and it would be nice to tidy this up.

In this branch, I made the interpreter introspect __exit__ and apply the single-arg API if it is either a method with co_argcount=2 or a C Function with flags == METH_O. In all other cases it applies the 3-arg api.

I added a bytecode for the no-exception exit case (previously we pushed 3 Nones and called __exit__, but now we don't know at compile time how many Nones we will need).

Benchmark results:

  1. Just adding the new bytecode had no impact.

  2. Also introspecting the function in each __exit__ call was 1% slower. This can be mitigated by: (*) specialising so that introspection is done only once. (2) we will get some of it back once the stdlib's __exit__s are rewritten to take one arg.

gvanrossum commented 1 year ago

Specialization to the rescue!

iritkatriel commented 1 year ago

I've created a branch with all stdlib context managers converted to single-arg __exit__, and it gives back the 1% we lost due to introspection. So with specialization this should give some speedup.

Although, note that we can't just convert the stdlib context managers because that can break code that calls __exit__ directly, or which subclasses an stdlib context manager.

markshannon commented 1 year ago

My suggestion is this:

The VM should always call __leave__

object.__leave__ and object.__exit__ will presumably be written in C, but here's the semantics in Python:

class object:

    def __exit__(self, type, value, traceback):
        leave = type(self).__leave__
        if leave is object.__leave__:
            raise TypeError(f"'{type(self)}' object does not support the context manager protocol")
        return leave(self, value)

    def __leave__(self, exc):
        return type(self).__exit__(self, type(exc), exc, exc.__traceback__)

This supports the old protocol, __exit__ and provides a small carrot of performance for third-party code to move from __exit__ to __leave__.

I believe this is fully backwards compatible, as dunder names are reserved for the language.

iritkatriel commented 1 year ago

I like this approach. Just one question - what should we do to make this work?

BaseCM:
   def __leave__(self, exc): ...

CM(BaseCM):
  def __exit__(self, exc, val, tb): ...

The interpreter should call object.__leave__, not BaseCM._leave__.

markshannon commented 1 year ago

Good point.

We could do something like we do for __eq__ and __hash__ and insert a __leave__ method for all classes that define __exit__, and an __exit__ method for classes that define __leave__. The generated __leave__ method would call the __exit__ method. Likewise, the generated __exit__ method would call the __leave__ method.

It is more complicated than my earlier suggestion, but should handle inheritance properly. It also has the advantage that testing for the presence of an __exit__ method to determine if something is a context manager will still work correctly.

OOI, have you looked at the prevalence of user defined __exit__ methods in the top 4000 projects?

iritkatriel commented 1 year ago

OOI, have you looked at the prevalence of user defined exit methods in the top 4000 projects?

Not yet, only the stdlib.

iritkatriel commented 1 year ago

There isn't currently an object.__exit__ slot - it's there if it's there. We need to somehow attach an __exit__ whenever someone defines a __leave__, and vice versa. We can't even do it at type creating time because someone could be doing this with monkey patching.

gvanrossum commented 1 year ago

Everything I was trying to say has already been said. We should carefully write up some examples involving inheritance and library evolution and see if a pattern emerges.

iritkatriel commented 1 year ago

The problem is what happens when an object has both __leave__ and __exit__.

If the interpreter calls __leave__ then we have a problem when there is a subclass that overrides an stdlib context manager's __exit__, and we've changed the base context manager to use __leave__. I think this is a big problem, so we would need to make the interpreter prefer __exit__ if it exists. This would mean that a new subclass that wants to implement __leave__ needs to be aware of whether the superclass implements __leave__ or __exit__.

(Unless we want to go into deeper introspection that determines which of __leave__ and __exit__ was defined where in the hierarchy.)

encukou commented 1 year ago

How about making it so that assigning to one of them automatically assigns an adapter to the other? The last one wins. Class creation is a sequence of attribute assignments, as far as these are concerned.

iritkatriel commented 1 year ago

How about making it so that assigning to one of them automatically assigns an adapter to the other? The last one wins. Class creation is a sequence of attribute assignments, as far as these are concerned.

We would need to check the name of every attribute assignment, to the class as well as to the object after it was created, to see if it's one of these two.

iritkatriel commented 1 year ago

How about making it so that assigning to one of them automatically assigns an adapter to the other? The last one wins. Class creation is a sequence of attribute assignments, as far as these are concerned.

We would need to check the name of every attribute assignment, to the class as well as to the object after it was created, to see if it's one of these two.

Actually we don't need to worry about monkey patching, because __exit__ is looked up with _PyObject_LookupSpecial, which looks for it on the type and not the instance. So maybe there is hope for this.

iritkatriel commented 1 year ago

I got into a pretty deep rabbit hole trying to implement the __exit__/__leave__ solution. It's all pretty straightforward until you try to make this work:

class B:
   def __enter__(self): pass
   def __leave__(self, exc):
       print('B.__leave__:', repr(exc), type(exc))

class D(B):
    def __exit__(self, typ, val, tb):
        print('D.__exit__:', type, val, tb)
        B.__exit__(self, type, val, tb)

e = TypeError(42)
D().__exit__(type(e), e, e.__traceback__)

I am returning to the introspection idea, because I think it is much simpler.

The only concrete concern raised w.r.t. my previous implementation was about def __exit__(self, typ, *arg) being mistaken for the single-arg version, but I've fixed that by checking whether (code->co_flags & CO_VARARGS).

gvanrossum commented 1 year ago

Agreed.

iritkatriel commented 1 year ago

Draft PR: https://github.com/python/cpython/pull/101995

iritkatriel commented 1 year ago

First draft of a PEP: https://github.com/python/peps/pull/3018

markshannon commented 1 year ago

I really think we should stick with the __leave__ approach. It is simpler to explain and understand. E.g. Is this a one or three arg __exit__?

def __exit__(self, exc, cache_value_in_default1 = ..., cache_value_in_default2 = ...):
    ...

The __leave__ approach isn't that hard to implement AFAICT. Something like this:

def make_exit(leave):
    def __exit__(self, et, ev, tb):
        return leave(self, ev)
    return __exit__

def make_leave(exit):
    def __leave__(self, ev):
        return exit(self, type(ev), ev, ev.__traceback__)
    return __leave__

#Define this as a metaclass here, but we will add this behavior to type.
class AddExitOrLeave(type):

    def __new__(meta, cls, bases, members):
        if "__leave__" in members:
            if "__exit__" not in members:
                members["__exit__"] = make_exit(members["__leave__"])
        elif "__exit__" in members:
            members["__leave__"] = make_leave(members["__exit__"])
        return type(cls, bases, members)

Which gives the desired behavior:

class B(metaclass=AddExitOrLeave):

    def __enter__(self):
        print("B.__enter__")
        return self

    def __leave__(self, exc):
        print("B.__leave__", exc)

class D(B, metaclass=AddExitOrLeave):

    def __exit__(self, et, ev, tb):
        print('D.__exit__:', et, ev, tb)
        B.__exit__(self, et, ev, tb)

print("\nCall B.__exit__ directly")
B().__exit__(Exception, Exception("message"), None)

print("\nCall B.__exit__ in with. No exception")
with B():
    pass

print("\nCall B.__exit__ in with. Raise exception")
try:
    with B():
        1/0
except:
    pass

print("\nCall D.__exit__ directly")
D().__exit__(Exception, Exception(), None)

print("\nCall D.__exit__ in with. No exception")
with D():
    pass

print("\nCall D.__exit__ in with. Raise exception")
try:
    with D():
        1/0
except:
    pass

print("\nCall D.__leave__ directly")
D().__leave__(Exception("message"))
iritkatriel commented 1 year ago

I really think we should stick with the __leave__ approach. It is simpler to explain and understand.

My problem was with implementing it, the descriptor approach was getting very complicated. If there is a simple implementation then I agree we should revisit this idea.

E.g. Is this a one or three arg __exit__?

def __exit__(self, exc, cache_value_in_default1 = ..., cache_value_in_default2 = ...):
    ...

In the proposal I wrote up this is 3-arg. If it is unambiguously 1-arg then it is, otherwise it's 3 args. It's not a perfect scheme, but it is simple.

iritkatriel commented 1 year ago

On second thought I think the __leave__ approach is different from what I wrote up in an important way - I am proposing to enhance the recogonized signatures of __exit__, while __leave__ is introducing a new dunder. Putting aside how this new dunder will be supported, do we even want a new dunder in the language spec, which does the same as another dunder but with a slightly different signature?

encukou commented 1 year ago

FWIW, I prefer __leave__: although it's complex, it better isolates complexity in the interpreter, rather than exposing sharp edge cases to third-party code. Specifically: If I maintain a library (or alternate Python implementation) that can call __exit__ on arbitrary objects, I'll need to exactly replicate how CPython decides which version to call. In turn, this means either CPython can't ever change that algorithm, or the third-party libraries are expected to follow any (potentially subtle) changes. CPython could provide a call_exit-style helper, but then we're heading back into expanding the API, which is AFAIK the major argument against __leave__.

With __leave__ it should be possible to avoid guessing. Having two similar things is a bit annoying, but on the other hand, __exit__ would be relatively easy to deprecate/remove later, as it'll be very clear that a given project still uses it.

iritkatriel commented 1 year ago

Thank you @encukou . I hope that we can have both implementations side by side for the core devs and SC to compare, perhaps two competing PEPs. Are you volunteering to implement this version?

With __leave__ it should be possible to avoid guessing. Having two similar things is a bit annoying, but on the other hand, __exit__ would be relatively easy to deprecate/remove later, as it'll be very clear that a given project still uses it.

I see where you're coming from, but while scanning code (stdlib as well as general GitHub code), the vast majority of the __exit__ function are a variation of

def __exit__(self, *args):
       do_something_that does_not_invovle_args()

and in many cases the something to do is "pass".

There will be no incentive to change this to

def __leave__(self, arg):
       do_something_that does_not_invovle_arg()

other than to silence the deprecation warning.

On the other hand, if we go with the introspection proposal, we can decide at some point to stop supporting the 3-arg version, then the __exit__ function above will just continue working.

So the introspection solution causes some more work for people who need to emulate python (a small number of users) but makes the transition seamless for many other users, and saves everyone learning a new dunder name.

markshannon commented 1 year ago

Three more reasons why __leave__ is better.

__leave__ is easier to explain. I suspect that the majority of people implementing a context manager will need to look up how to do it in the documentation, or in a tutorial. For them, it doesn't matter if the method is called __leave__ or __exit__.

The original motivation for this change was performance. Having to introspect the __exit__ function is slower than just calling __leave__ (although the introspection could hypothetically be done when specializing).

If it is not clear to developers whether __exit__ will be called with one or three arguments (e.g. they need to support versions before and after the removal of support for the three argument form) then they are going to have to write stuff like this:

def __exit__(self, arg1, arg2=SENTINEL, arg2=SENTINEL):
    if arg2 is SENTINEL:
        exc = arg1
    else:
        exc = arg2
    ...
iritkatriel commented 1 year ago

The original motivation for this change was performance.

Not really. It was to simplify the language. Performance is a bonus in this case.

(e.g. they need to support versions before and after the removal of support for the three argument form) then...

they can just keep the 3-arg signature until they are ready to drop support for 3 args. There is no requirement to support a single-arg signature if you still need the 3-arg one.

markshannon commented 1 year ago

So, support for the three argument form will never be removed?

iritkatriel commented 1 year ago

So, support for the three argument form will never be removed?

Certainly not while there are maintained python versions that only support 3 args. Maybe in 5-10 years it will be feasible to talk about this. (But this is true for whatever solution we choose).

markshannon commented 1 year ago

When it is removed doesn't matter. But there will two versions, 3.M that supports the 3 argument form, and 3.N that only supports the one argument form.

Now, suppose I'm a library author and I want to support 3.M and 3.N. If we add __leave__, I need to write:

def __leave__(exc):
   ...

That's all. The 3.M VM will handle __exit__ for me, and in 3.N __exit__ won't exist.

But with one-argument __exit__, I need to write this:

def __exit__(self, arg1, arg2=SENTINEL, arg2=SENTINEL):
    if arg2 is SENTINEL:
        exc = arg1
    else:
        exc = arg2
    ...

because I cannot be sure that a subclass won't call super().__exit__(et, ev, tb) in 3.M.

This is just one corner case. There will be others because, in Python, the arity of a function is not a consideration when calling a function; the function has to handle the differing arity with default values.

iritkatriel commented 1 year ago

because I cannot be sure that a subclass won't call super().__exit__(et, ev, tb) in 3.M.

That subclass wouldn't work with the __leave__ solution either once we remove __exit__.

encukou commented 1 year ago

Are you volunteering to implement this version?

It sounds like a project I'd enjoy, but sadly, I don't think I can fit it in my schedule for 3.12 :(

markshannon commented 1 year ago

That subclass wouldn't work with the __leave__ solution either once we remove __exit__

True, but it isn't the subclass that's the problem. It is the potential existence of the subclass that is a problem for the superclass.

iritkatriel commented 1 year ago

That subclass wouldn't work with the __leave__ solution either once we remove __exit__

True, but it isn't the subclass that's the problem. It is the potential existence of the subclass that is a problem for the superclass.

Most context managers just have def __exit__(*args) and ignore the args, so they won't need to care about subclasses in the introspection solution, but with the __leave__ solution they need to provide both __leave__ and __exit__ if we are ever going to drop __exit__.

Those that do use the args in __exit__ will have to handle both signatures one way or another (in one function with defaults, or in two functions).

markshannon commented 1 year ago

but with the __leave__ solution they need to provide both __leave__ and __exit__ if we are ever going to drop exit.

No, they just need to provide __leave__. type provides the __exit__function for versions that still support __exit__.

iritkatriel commented 1 year ago

No, they just need to provide __leave__. type provides the __exit__function for versions that still support __exit__.

Forever?

markshannon commented 1 year ago

Forever, they just need to provide __leave__?

Yes, that's how you define a context manager.

Forever, type provides the __exit__function for versions that still support __exit__?

No, only for versions that still support __exit__.

iritkatriel commented 1 year ago

If someone calls super().__exit__() from their __exit__, and the super() class has only __leave__, that works as long as python adds the missing __leave__ and __exit__. Once we remove that from python it malfunctions but seems to work, because the interpreter will just call the superclass __leave__ and bypass the override.

It would be better to get a type error on wrong number of args, which is what would happen if we make __exit__ suddenly support only one arg when you expect 3.

markshannon commented 1 year ago

Once we remove that from python it malfunctions but seems to work, because the interpreter will just call the superclass leave and bypass the override.

True, which is why we have deprecation warnings. We can even make the existence of a __exit__ method an error; it's a cheap check.

iritkatriel commented 1 year ago

Closing as we've done what we can and it's now in the hands of the SC.