Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not always use exceptions to return result from coroutine #85922

Closed
vladima mannequin opened this issue Sep 11, 2020 · 49 comments
Closed

Do not always use exceptions to return result from coroutine #85922

vladima mannequin opened this issue Sep 11, 2020 · 49 comments
Labels
3.10 only security fixes performance Performance or resource usage

Comments

@vladima
Copy link
Mannequin

vladima mannequin commented Sep 11, 2020

BPO 41756
Nosy @scoder, @encukou, @asvetlov, @ambv, @markshannon, @serhiy-storchaka, @1st1, @vladima
PRs
  • bpo-41756: Introduce _PyGen_SendNoStopIteration to provide alternative way to return results from coroutines #22196
  • bpo-41756: Refactor gen_send_ex(). #22330
  • bpo-41756: Add PyIter_Send function to allow sending value into generators/coroutines/iterators #22443
  • bpo-41756: Delete PyGen_Send function #22663
  • bpo-41756: Export PyGen_Send and wrap it in if-defs #22677
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-05-05.10:13:43.239>
    created_at = <Date 2020-09-11.01:10:36.924>
    labels = ['3.10', 'performance']
    title = 'Do not always use exceptions to return result from coroutine'
    updated_at = <Date 2021-07-23.16:04:57.057>
    user = 'https://github.com/vladima'

    bugs.python.org fields:

    activity = <Date 2021-07-23.16:04:57.057>
    actor = 'petr.viktorin'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-05-05.10:13:43.239>
    closer = 'asvetlov'
    components = []
    creation = <Date 2020-09-11.01:10:36.924>
    creator = 'v2m'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41756
    keywords = ['patch']
    message_count = 49.0
    messages = ['376698', '376701', '376703', '376721', '376735', '376737', '377001', '377002', '377003', '377006', '377007', '377008', '377023', '377025', '377051', '377063', '377066', '377067', '377077', '377092', '377097', '377098', '377109', '377128', '377132', '377136', '377137', '377146', '377147', '377215', '377219', '377249', '377266', '377298', '377299', '377582', '377587', '377608', '378357', '378379', '378425', '378432', '378515', '378518', '378527', '378570', '382003', '398074', '398075']
    nosy_count = 8.0
    nosy_names = ['scoder', 'petr.viktorin', 'asvetlov', 'lukasz.langa', 'Mark.Shannon', 'serhiy.storchaka', 'yselivanov', 'v2m']
    pr_nums = ['22196', '22330', '22443', '22663', '22677']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue41756'
    versions = ['Python 3.10']

    @vladima
    Copy link
    Mannequin Author

    vladima mannequin commented Sep 11, 2020

    Currently async functions are more expensive to use comparing to their sync counterparts. A simple microbenchmark shows that difference could be quite significant:

    import time
    
    def f(a):
        if a == 0:
            return 0
        return f(a - 1)
    
    async def g(a):
        if a == 0:
            return 0
        return await g(a - 1)
    
    N = 100000
    C = 200
    
    t0 = time.time()
    for _ in range(N):
        f(C)
    t1 = time.time()
    for _ in range(N):
        try:
            g(C).send(None)
        except StopIteration:
            pass
    t2 = time.time()
    
    print(f"Sync functions: {t1 - t0} s")
    print(f"Coroutines: {t2 - t1} s")
    

    Results from master on my machine:

    Sync functions: 2.8642687797546387 s
    Coroutines: 9.172159910202026 s

    NOTE: Due to viral nature of async functions their number in codebase could become quite significant so having hundreds of them in a single call stack is not something uncommon.

    One of reasons of such performance gap is that async functions always return its results via raising StopIteration exception which is not cheap. This can be avoided if in addition to _PyGen_Send always return result via exception we could have another function that will allow us to distinguish whether value that was returned from generator is a final result (return case) of whether this is yielded value.
    In linked PR I've added function _PyGen_SendNoStopIteration with this behavior and updated ceval.c and _asynciomodule.c to use it instead of _PyGen_Send which resulted in a measurable difference:

    Sync functions: 2.8861589431762695 s
    Coroutines: 5.730362176895142 s

    @vladima vladima mannequin added 3.10 only security fixes performance Performance or resource usage labels Sep 11, 2020
    @1st1
    Copy link
    Member

    1st1 commented Sep 11, 2020

    Big +1 from me. This is something I always wanted to do myself (since the time of PEP-492 & 525 implementations) and I think this is a necessary change. It's great that this isn't just a C API UX improvement but also yields a big perf improvement.

    @scoder
    Copy link
    Contributor

    scoder commented Sep 11, 2020

    Big +1 from me, too, for the same reasons Yury gave.

    @scoder
    Copy link
    Contributor

    scoder commented Sep 11, 2020

    Copying some of the design discussion from the PR here (https://github.com/python/cpython/pull/22196/files#r486730457), because it belongs into the ticket.

    Yury Selivanov proposed to add a new C-API function for this (naming changes by me):

    typedef enum {PYGEN_RETURN, PYGEN_ERROR, PYGEN_NEXT} PyGenSendStatus;
    
        PyGenSendStatus PyGen_Send(PyGenObject *gen, PyObject *arg, PyObject **result);

    Mark Shannon and I agreed that the status code should be the return value, with some confusion whether "PyGen_" or "PyCoro_" would be appropriate prefixes.

    Mark Shannon wrote: I don't think [the C-API function] should be public, as a possible further improvement is to stop passing exceptions through a side channel, but in result. Maybe we don't want to do that, but lets' not add to the (already rather large) C-API.

    However, I think this will be demanded and used by extensions, including Cython implemented ones, so it seems better to make them use a public function than a private one.

    Let's continue these lines of discussion here.

    @vladima
    Copy link
    Mannequin Author

    vladima mannequin commented Sep 11, 2020

    If I understand proposed shape of API correctly - it was not supposed to return exception via "result" so contract for new PyGen_Send function is something like:

    Return value | result | Comment
    -----------------------------------------------------
    PYGEN_RETURN | not NULL | Returned value
    PYGEN_NEXT | not NULL | Yielded value
    PYGEN_ERROR | NULL | Regular PyErr_* functions should be used to work with error case

    @1st1
    Copy link
    Member

    1st1 commented Sep 11, 2020

    @mark

    Mark Shannon wrote: I don't think [the C-API function] should be public, as a possible further improvement is to stop passing exceptions through a side channel, but in result. Maybe we don't want to do that, but lets' not add to the (already rather large) C-API.

    Yeah, we can add it as a "private" function, I'm not entirely opposed to that. But... it would be great if Cython and C code could still depend on it and use it. And then... why should it be private? The corresponding Python API "gen.send()" and "gen.throw()" is public, why can't the C API be public too?

    We will not fundamentally change generators (it would be a major backwards incompatible change), so committing to a good C API sounds reasonable.

    @mark

    Remember that PyIter_Next() is pretty much the same, though, and it has the standard "return PyObject*" interface. These two would diverge then.

    Maybe we should call it _PyIter_Send()? While .send() is mostly about coroutines, regular generators have the method too, and it would be weird to call _PyCoro_Send on a generator object.

    @vladimir

    PYGEN_ERROR | NULL | Regular PyErr_* functions should be used to work with error case

    Correct.

    @1st1
    Copy link
    Member

    1st1 commented Sep 16, 2020

    Mark, Stefan,

    I don't want this to be stale so I propose to move with my suggestions:

    1. We make the new API public. Mark, if you have objections to that - please elaborate with some details. IMO, the corresponding Python API is long public and there's no harm in exposing a C version of it. Especially given the fact that uvloop, cython, and even asyncio itself will be relying on that API.

    2. I propose to name the new API PyIter_Send. Motivation: it will work with both generators and coroutines and plays nicely with PyIter_Next.

    @serhiy-storchaka
    Copy link
    Member

    As for returned value, I propose to return -1 in case of error, 1 for yielded value and 0 for returned value (i.e. define PYGEN_RETURN = 0, PYGEN_YIELD = 1 and PYGEN_ERROR = -1, but without exposing public names).

    It would be uniform with other C API: many functions return -1 on error (if they return int and can fail), and PyDict_Next() and _PySet_NextEntry() return 1 for every yielded item, and 0 if the iteration has been finished.

    @1st1
    Copy link
    Member

    1st1 commented Sep 16, 2020

    As for returned value, I propose to return -1 in case of error, 1 for yielded value and 0 for returned value (i.e. define PYGEN_RETURN = 0, PYGEN_YIELD = 1 and PYGEN_ERROR = -1, but without exposing public names).

    Sure, that works.

    @scoder
    Copy link
    Contributor

    scoder commented Sep 16, 2020

    I'm happy to see this moving forward.

    Not convinved of the "PyIter_Send()" name, though. I don't consider this part of the iterator protocol. It's specific to generators and coroutines. Cython would probably guard its usage by "PyGen_CheckExact()" or "PyCoro_CheckExact()", and not use it for arbitrary iterators.

    Since coroutines inherit the generator protocol more or less, I think "PyGen_Send()" is a more suitable name, better than "PyCoro_Send()".

    @vladima
    Copy link
    Mannequin Author

    vladima mannequin commented Sep 16, 2020

    Also should it be specific to generators/coroutines and accept PyGenObject* or should it try to handle multiple cases and expose the result for them in uniform way, i.e.

    if (PyGen_CheckExact(gen) || PyCoro_CheckExact(gen)) {
       // use coroutine/generator specific code that avoids raising exceptions
       *result = ...
       return PYGEN_RETURN;
    }
    PyObject *ret;
    if (arg == Py_None) {
      ret = Py_TYPE(gen)->tp_iternext(gen);
    }
    else {
      ret = _PyObject_CallMethodIdOneArg(coro, &PyId_send, arg);
    }
    if (ret != NULL) {
      *result = ret;
      return PYGEN_YIELD;
    }
    if (_PyGen_FetchStopIterationValue(result) == 0) {
      return PYGEN_RETURN;
    }
    return PYGEN_ERROR;
    

    @1st1
    Copy link
    Member

    1st1 commented Sep 16, 2020

    Since coroutines inherit the generator protocol more or less, I think "PyGen_Send()" is a more suitable name, better than "PyCoro_Send()".

    OK, +1. PyGen_Send it is.

    Also should it be specific to generators/coroutines and accept PyGenObject*

    I think it should be specific to generators and coroutines. Calling PyObject_CallMethodIdOneArg(coro, &PyId_send, arg); and interpreting exceptions to emulate the low level API seems a bit too much.

    @1st1
    Copy link
    Member

    1st1 commented Sep 16, 2020

    I think it should be specific to generators and coroutines. Calling PyObject_CallMethodIdOneArg(coro, &PyId_send, arg); and interpreting exceptions to emulate the low level API seems a bit too much.

    To add to my point: typically higher-level APIs go under the PyObject_* namespace, whereas Py{Type}_* is more concrete. So I'd make PyGen_Send to only work with PyGen and PyCoro.

    @serhiy-storchaka
    Copy link
    Member

    There are other abstract object APIs: PyNumber, PySequence, PyMapping, etc. In particularly PyIter_Next() works with the iterator protocol, there is no single iterator class. Seems PyGen_* API is related to concrete class, but we can introduce new namespace for the generator protocol.

    @markshannon
    Copy link
    Member

    I agree with Serhiy, that PyGen_ is a bad prefix.

    Unless a function takes generator objects and only generators objects, then it shouldn't have a PyGen prefix.

    The API function is the C equivalent of obj.send(val).
    The first parameter is an object.

    Coroutines do not inherit from generators.
    That the the C implementations are so coupled is an unfortunate historical accident, and may well be changed.

    Regardless of how this is implemented internally, any API function's signature should reflect the equivalent Python code.
    And please use an enum, not an int, as the return value. It's a huge boon for readability.

    I would suggest:
    PySendResult PyIter_Send(PyObject *obj, PyObject *arg, PyObject **result);

    @vladima
    Copy link
    Mannequin Author

    vladima mannequin commented Sep 17, 2020

    I guess PyIter_Send would imply that this function should work for all inputs (like in https://bugs.python.org/msg377007) which also sounds reasonable.

    @1st1
    Copy link
    Member

    1st1 commented Sep 17, 2020

    I guess PyIter_Send would imply that this function should work for all inputs (like in https://bugs.python.org/msg377007) which also sounds reasonable.

    I like PyIter_Send (and why I initially proposed it myself) because it will also work with the "send" slot defined on some types if we end up adding one.

    @vladima
    Copy link
    Mannequin Author

    vladima mannequin commented Sep 17, 2020

    so to summarize:

    Proposed function signature:

    PySendResult PyIter_Send(PyObject *obj, PyObject *arg, PyObject **result);
    

    For generators/coroutines function will delegate to specialized implementation that does not raise StopIteration exception
    For types that provide tp_iternext if arg is Py_None function call invoke Py_TYPE(obj)->tp_iternext(obj)
    For all other cases function will try to call send method

    Regarding of the case function will not raise StopIteration and will always return pair status/result.

    Does it sound correct?

    @1st1
    Copy link
    Member

    1st1 commented Sep 18, 2020

    Does it sound correct?

    It does, but given the amount of back and forth on this, I'd wait for Serhiy and Stefan to confirm if they're OK.

    IMO the PyIter_Send name is OK (given how generic the implementation will be) and returning an enum, while a tad unconventional for the C API is way more convenient for the developer.

    @serhiy-storchaka
    Copy link
    Member

    I would be fine even with a generator-specific API as a first step, for simplicity. But the end goal is to support all generator-like objects. It is much more useful for end users and does not have drawbacks.

    Enum for result seems not necessary (other functions with three-state result just return -1, 0 or 1), but if other core developers want it -- OK.

    @scoder
    Copy link
    Contributor

    scoder commented Sep 18, 2020

    I would also have preferred a more type specific function, but yeah, as long as the types for which the function would normally be used are special cased early enough in the implementation, it makes no big difference.

    Fine with me, too.

    @scoder
    Copy link
    Contributor

    scoder commented Sep 18, 2020

    BTW, just to give this a house number, I remember having measured a performance improvement of up to 70% at some point when switching from "generators always raise a StopIteration at the end" to "generators just return NULL" in Cython. For short-running generators and coroutines, this can make a big difference.

    @serhiy-storchaka
    Copy link
    Member

    We introduced _PyObject_LookupAttr() and _PyObject_GetMethod() for similar purposes. And they have similar signatures. Although they all are private.

    @1st1
    Copy link
    Member

    1st1 commented Sep 18, 2020

    I would also have preferred a more type specific function, but yeah, as long as the types for which the function would normally be used are special cased early enough in the implementation, it makes no big difference.

    Maybe add two API funcs: PyGen_Send (specific to generators & coroutines) and PyIter_Send?

    @vladima
    Copy link
    Mannequin Author

    vladima mannequin commented Sep 18, 2020

    Sounds like a good middleground to start: add PySendResult and PySendResult PyGen_Send(PyGenObject*, PyObject* PyObject**) specific to generators and coroutines. Subsequent changes could introduce PySendResult PyIter_Send(PyObject*, PyObject*, PyObject**) that would be more generic (call tp_next, invoke "send" or maybe in the future use dedicated slot for "send" operator so i.e. asyncio.Future or Cython coroutines could benefit from the same optimization)

    @1st1
    Copy link
    Member

    1st1 commented Sep 18, 2020

    Sounds like a good middleground to start: add PySendResult and PySendResult PyGen_Send(PyGenObject*, PyObject* PyObject**) specific to generators and coroutines.

    Yes, it seems that everybody agreed on that. I can give the PR another review -- is it ready?

    @1st1 1st1 closed this as completed Sep 19, 2020
    @serhiy-storchaka
    Copy link
    Member

    PR 22330 refactors gen_send_ex(), making it similar to the new PyGen_Send().

    @1st1
    Copy link
    Member

    1st1 commented Sep 20, 2020

    Vladimir, could you please submit a PR to update 3.10/whatsnew? Need to mention both the new C API and the new perf boost in relevant sections.

    @markshannon
    Copy link
    Member

    Yury,

    Why was the PR merged with a new API function PyGen_Send?

    I explicitly said that any new API function should not start with PyGen, nor should any function rely on generators and async "coroutines" sharing the same memory layout.

    If you disagree with me, please say why, don't just merge the PR.

    The name PyGen is misleading as it can handle coroutines as well as generators.
    There is no performance advantage to only handling these two types.
    Worse, it requires that a PyCoroObject can always be cast to a PyGenObject, preventing better layout of either object in the future.

    Would you revert the PR, please.

    @1st1
    Copy link
    Member

    1st1 commented Sep 21, 2020

    If you disagree with me, please say why, don't just merge the PR.

    Apologies, Mark. I didn't intend to merge something bypassing your opinion; just missed your comment between reviewing multiple PRs in a few unrelated repos. I'm sorry.

    On the actual naming subject, you proposed:

    PySendResult PyIter_Send(PyObject *obj, PyObject *arg, PyObject **result);

    The problem with using this name is that ideally we should also support non-native coroutine and generator implementations (i.e. resolve the "send" attribute and call it using Python calling convention). Ideally we should have two C APIs: one low-level supporting only native objects and a high level one, supporting all kinds of them.

    Can we perhaps add both PyGen_Send() and PyCoro_Send() for now that would only accept generators and coroutines respectively? After that we can discuss adding a more generic PyIter_Send?

    Would you revert the PR, please.

    Since this is in 3.10/master that nobody uses right now except us (Python core devs), can we just issue a follow up PR to fix whatever is there to fix? I'd like to avoid the churn of reverting, and again, I apologize for pushing this a bit hastily. Let me know if you actually want a revert and I'll do that.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 6c33385 by Serhiy Storchaka in branch 'master':
    bpo-41756: Refactor gen_send_ex(). (GH-22330)
    6c33385

    @serhiy-storchaka
    Copy link
    Member

    I agree that we should add PyIter_Send() (the name is discussable, but PyIter_Send LGTM) which should support iterators and objects with the send() method. It would be much more useful for user, and can replace PyGen_Send() in the interpreter core code. But merging the PR with PyGen_Send() was not a mistake. It was good to split the changes on smaller steps easier to review.

    Vladimir, do you mind to create a new PR for PyIter_Send()? I seen its implementation in one of intermediate versions of your PR.

    @vladima
    Copy link
    Mannequin Author

    vladima mannequin commented Sep 27, 2020

    Serhiy, AFAIR PyIter_Send in my PR appear only as a rename from placeholder Name_TBD and it still was specific to PyGenObjects. Do you mean something that was listed in https://bugs.python.org/msg377007 ?

    @serhiy-storchaka
    Copy link
    Member

    No, I meant a function which combines PyGen_Send, tp_iternext and _PyGen_FetchStopIterationValue. Was not it in your PR?

    @vladima
    Copy link
    Mannequin Author

    vladima mannequin commented Sep 28, 2020

    No, I don't think so but I can definitely make one. A few questions first:

    • having PySendResult as a result type of PyIterSend seems ok, however prefix for each concrete value (PYGEN_*) is not aligned with the prefix of the function itself (PyIter_)
    • should it also deal with tstate->c_tracefunc (probably not) or just be something like
    PySendResult
    PyIter_Send(PyObject *iter, PyObject *arg, PyObject **result)
    {
        _Py_IDENTIFIER(send);
        assert(result != NULL);
    
        if (PyGen_CheckExact(iter) || PyCoro_CheckExact(iter)) {
            return PyGen_Send((PyGenObject *)iter, arg, result);
        }
    
        if (arg == Py_None && Py_TYPE(iter)->tp_iternext != NULL) {
            *result = Py_TYPE(iter)->tp_iternext(iter);
        }
        else {
            *result = _PyObject_CallMethodIdOneArg(iter, &PyId_send, arg);
        }
    
        if (*result == NULL) {
            if (_PyGen_FetchStopIterationValue(result) == 0) {
                return PYGEN_RETURN;
            }
            return PYGEN_ERROR;
        }
        return PYGEN_NEXT;
    }
    

    @1st1
    Copy link
    Member

    1st1 commented Oct 10, 2020

    New changeset 037245c by Vladimir Matveev in branch 'master':
    bpo-41756: Add PyIter_Send function (bpo-22443)
    037245c

    @markshannon
    Copy link
    Member

    Vladimir,
    Thanks for adding PyIter_Send().

    Don't forget to remove PyGen_Send() :)

    @scoder
    Copy link
    Contributor

    scoder commented Oct 11, 2020

    Don't forget to remove PyGen_Send()

    That's the function that allows sending data into a generator. It's also used internally by "PyIter_Send()". Are you really suggesting to remove it, or to make it underscore-private again? (As it was before.)

    @markshannon
    Copy link
    Member

    What's the difference between removing it and making it properly private (i.e. static)?

    It's an irrelevant detail whether the code is inlined or in a helper function.

    @1st1
    Copy link
    Member

    1st1 commented Oct 12, 2020

    With the latest PR now merged this issue can be closed. Please reopen if there are any other action items left.

    @1st1 1st1 closed this as completed Oct 12, 2020
    @1st1 1st1 closed this as completed Oct 12, 2020
    @serhiy-storchaka
    Copy link
    Member

    Few things I forget about. The new C API function should be exported in PC/python3dll.c.

    Also, in Include/abstract.h it should only be available for limited C API >= 3.10:

    #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030A0000
    ...
    #endif

    We now should discuss the behavior of PySend_Iter() if value is NULL. It is not documented, the current behavior differs from the behavior for non-NULL value, and it is used in the ceval loop. We should document this case explicitly and maybe change the behavior if it would be more appropriate.

    @1st1 1st1 reopened this Oct 12, 2020
    @1st1 1st1 reopened this Oct 12, 2020
    @1st1
    Copy link
    Member

    1st1 commented Oct 12, 2020

    Also, in Include/abstract.h it should only be available for limited C API >= 3.10:

    Vladimir, could you please submit a PR?

    We now should discuss the behavior of PySend_Iter() if value is NULL. It is not documented, the current behavior differs from the behavior for non-NULL value, and it is used in the ceval loop.

    IMO: I'd keep the behavior and just document it.

    @1st1
    Copy link
    Member

    1st1 commented Oct 13, 2020

    New changeset cfb0f57 by Vladimir Matveev in branch 'master':
    bpo-41756: Export PyGen_Send and wrap it in if-defs (bpo-22677)
    cfb0f57

    @asvetlov
    Copy link
    Contributor

    Can we close the issue?

    @encukou
    Copy link
    Member

    encukou commented Jul 23, 2021

    Hello!
    This change added an enum to the stable ABI:
    typedef enum {
    PYGEN_RETURN = 0,
    PYGEN_ERROR = -1,
    PYGEN_NEXT = 1,
    } PySendResult;

    Adding new values to enums might break the stable ABI in some (admittedly rare) cases; so usually it's better to avoid enums and stick with int and #ifdef'd values.
    This particular one looks like it won't be extended and won't cause problems, but still, switching to int would make stable ABI easier to work with.
    Is anyone against switching to int?

    See pbo-44727 for details.

    @encukou
    Copy link
    Member

    encukou commented Jul 23, 2021

    that is, bpo-44727

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants