Skip to content

Implement types._GeneratorWrapper in C #133372

Closed

Description

@efimov-mikhail

Feature or enhancement

Proposal:

I've found an old TODO line in types module and decided to give it a try:
https://github.com/python/cpython/blob/main/Lib/types.py#L253

My PR with this change will be linked soon.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

Activity

  1. sobolevn commented on May 4, 2025

    @sobolevn
    Member

    Please, before diving to deep into it: explain what we will achieve by doing so. For example: performance (and in what cases). Also: take extra care about potential compatibility changes.

  2. added
    stdlibStandard Library Python modules in the Lib/ directory
    on May 4, 2025
  3. added a commit that references this issue on May 4, 2025
  4. efimov-mikhail commented on May 4, 2025

    @efimov-mikhail
    MemberAuthor

    Please, before diving to deep into it: explain what we will achieve by doing so. For example: performance (and in what cases). Also: take extra care about potential compatibility changes.

    Actually, base PR is already present, I've linked it.
    In my opinion, this change is kinda similar to two things that we already have in codebase.
    First is basic collections that have implementations both in Python and C, OrderedDict for example.
    Pure Python implementation will not be removed but faster C implementation is added and it will be used if possible.

    Second is PyCoroWrapper in genobject.c which wraps generator object.
    IMO, _GeneratorWrapper is very similar to this class and can be implemented in C for performance.

    I don't think that performance gain is big, mainly because _GeneratorWrapper is only used in types.coroutine decorator.
    And that decorator is kinda old-style and not used much.
    But this is still an improvement in very basic class for async programming.

    I also slightly improve pure Python implementation of _GeneratorWrapper: gi_suspended and cr_suspended properties are added.
    IMO, they were missed by mistake.

    Speaking about compatibility, I can see no problems, because C version is very close to pure Python version.
    Only exception texts aren't equal and setting arbitrary values to __name__ / __qualname__ is prohibited as with general functions and generators.

  5. efimov-mikhail commented on May 4, 2025

    @efimov-mikhail
    MemberAuthor

    Moreover, there is another "TODO: Implement this in C." line in this file:

    # TODO: Implement this in C.

    It seems that types.coroutine also should be implemented both in C and in Python.
    I can make this change in separate PR if this one will be accepted.

  6. added and removed
    stdlibStandard Library Python modules in the Lib/ directory
    on May 4, 2025
  7. ZeroIntensity commented on May 4, 2025

    @ZeroIntensity
    Member

    I tried this a few months ago on my own, and from that experience, I'm really concerned about this change.

    • For one, _GeneratorWrapper isn't something that needs to be all that fast. In coroutines, the bottleneck is always the IO. I think any speedup we get will be negated by IO operations.
    • The linked PR seemed incorrect at a quick glance, so the complexity will likely only get worse (for example, the PR is missing thread-safety).
  8. picnixz commented on May 4, 2025

    @picnixz
    Member

    How fast are we in terms of import time however? Here it's interesting to see if we can improve the import time of types.

  9. sobolevn commented on May 4, 2025

    @sobolevn
    Member

    The linked PR seemed incorrect at a quick glance, so the complexity will likely only get worse (for example, the PR is missing thread-safety)

    I also fear that we might introduce some bugs with the rewrite. Since @types.coroutine is very outdated, I don't think that we should really touch it :(

    Unless we see a really good reasoning.

  10. AA-Turner commented on May 4, 2025

    @AA-Turner
    Member

    Since @types.coroutine is very outdated, I don't think that we should really touch it :(

    Would it be worth considering deprecating @types.coroutine?

    In the top 10,000 PyPI projects, it's only used in 91: amazon-ion, aspidites, async-generator, esphome, langgraph, nuitka, pycapnp, telnetlib3, and trio.

    Of these, the uses in aspidites, nuitka, and pycapnp are test-only.

    A

    Footnotes

    1. excluding asyncio and taskgroup, which are stdlib backports ↩

  11. sobolevn commented on May 4, 2025

    @sobolevn
    Member

    I think we can do that for 3.15, it would mark 10 versions of async def addition.
    But, asyncio team should decide on it :)

  12. AA-Turner commented on May 4, 2025

    @AA-Turner
    Member
  13. kumaraditya303 commented on May 5, 2025

    @kumaraditya303
    Contributor

    asyncio doesn't support generator based coroutines anymore so optimizing this isn't worth it. I think the comment should be removed as it is not relevant nowadays.

  14. efimov-mikhail commented on May 5, 2025

    @efimov-mikhail
    MemberAuthor

    Since @types.coroutine is very outdated, I don't think that we should really touch it :(

    Would it be worth considering deprecating @types.coroutine?

    IMO, this is very good question, and we could consider it in the details.
    FYI, 4 years ago there were reasons why not to deprecate it:

    #81102 (comment)

    I should learn these code snippets deeply to better understand the current situation.

  15. AA-Turner commented on May 5, 2025

    @AA-Turner
    Member

    also cc @njsmith. Both the examples cited by Nathaniel are 'simple' cases where all types.coroutine does is add a flag to co_flags, so another alternative would be to consider deprecating wrapping generator-like functions & _GeneratorWrapper, but keep the flag-mutation code.

    A

  16. efimov-mikhail commented on May 9, 2025

    @efimov-mikhail
    MemberAuthor
  17. kumaraditya303 commented on May 21, 2025

    @kumaraditya303
    Contributor

    I don't there is any benefit of changing or deprecating it, there are valid use cases for it and I think it's best to just remove the comment of implementing it in C and close this issue.

  18. added
    pendingThe issue will be closed if no feedback is provided
    on May 21, 2025
  19. added a commit that references this issue on May 23, 2025
  20. moved this from Todo to Done in asyncioon May 23, 2025
  21. added a commit that references this issue on Jul 12, 2025
  22. added a commit that references this issue on Aug 4, 2025
  23. added a commit that references this issue on Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    extension-modulesC modules in the Modules dirpendingThe issue will be closed if no feedback is providedtopic-asynciotype-featureA feature request or enhancement

    Projects

    • Status
      Done

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions