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

Handle generator (and coroutine) state in the bytecode. #87849

Closed
markshannon opened this issue Mar 31, 2021 · 17 comments
Closed

Handle generator (and coroutine) state in the bytecode. #87849

markshannon opened this issue Mar 31, 2021 · 17 comments
Assignees
Labels
3.10 only security fixes 3.11 only security fixes 3.12 bugs and security fixes performance Performance or resource usage

Comments

@markshannon
Copy link
Member

markshannon commented Mar 31, 2021

BPO 43683
Nosy @markshannon, @pablogsal, @sweeneyde, @dpgeorge
PRs
  • bpo-43683: Handle check for sending None to starting generator and coroutine in the bytecode. #25137
  • bpo-43683: Handle generator entry in bytecode #25138
  • bpo-43683: Minor corrections. #25224
  • bpo-43683: bump the bytecode magic number #25225
  • bpo-43683: Streamline YIELD_VALUE and SEND #30723
  • 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 = 'https://github.com/markshannon'
    closed_at = None
    created_at = <Date 2021-03-31.16:45:03.337>
    labels = ['3.11', '3.10', 'performance']
    title = 'Handle generator (and coroutine) state in the bytecode.'
    updated_at = <Date 2022-01-24.11:08:57.165>
    user = 'https://github.com/markshannon'

    bugs.python.org fields:

    activity = <Date 2022-01-24.11:08:57.165>
    actor = 'Mark.Shannon'
    assignee = 'Mark.Shannon'
    closed = False
    closed_date = None
    closer = None
    components = []
    creation = <Date 2021-03-31.16:45:03.337>
    creator = 'Mark.Shannon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 43683
    keywords = ['patch']
    message_count = 15.0
    messages = ['389919', '390305', '390354', '404564', '405081', '405083', '405096', '407983', '407987', '407988', '408006', '408009', '409725', '409751', '411465']
    nosy_count = 4.0
    nosy_names = ['Mark.Shannon', 'pablogsal', 'Dennis Sweeney', 'dpgeorge']
    pr_nums = ['25137', '25138', '25224', '25225', '30723']
    priority = None
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue43683'
    versions = ['Python 3.10', 'Python 3.11']

    Linked PRs

    @markshannon
    Copy link
    Member Author

    Every time we send, or throw, to a generator, the C code in genobject.c needs to check what state the generator is in.
    This is inefficient and couples the generator code, which should just be a thin wrapper around the interpreter, to the internals of the interpreter.

    The state of the generator is known to the compiler. It should emit appropriate bytecodes to handle the different behavior for the different states.

    While the main reason this is robustness and maintainability, removing the complex C code between Python caller and Python callee also opens up the possibility of some worthwhile optimizations.

    There are three changes I want to make:

    1. Add a new bytecode to handle starting a generator. This GEN_START bytecode would pop TOS, raising an exception if it is not None.
      This adds some overhead for the first call to iter()/send() but speeds up all the others.

    2. Handle the case of exhausted generators. This is a bit more fiddly, and involves putting an infinite loop at the end of the generator. Something like:

      CLEAR_FRAME
      label:
      GEN_RETURN (Like RETURN_VALUE None, but does not discard the frame)
      JUMP label

    This removes a lot of special case code for corner cases of exhausted generators and coroutines.

    1. Handle throw() on YIELD_FROM. The problem here is that we need to differentiate between exceptions triggered by throw, which must call throw() on sub-generators, and exceptions propagating out of sub-generators which should be passed up the stack. By splitting the opcode into two (or more), it is clear which case is being handled in the interpreter without complicated logic in genobject.c

    @markshannon markshannon self-assigned this Mar 31, 2021
    @markshannon markshannon added the performance Performance or resource usage label Mar 31, 2021
    @markshannon
    Copy link
    Member Author

    New changeset b37181e by Mark Shannon in branch 'master':
    bpo-43683: Handle generator entry in bytecode (GH-25138)
    b37181e

    @sweeneyde
    Copy link
    Member

    Looks like we both opened PRs in the same minute.

    The MAGIC constant didn't get updated, but perhaps that can just be included in the Minor Corrections PR.

    I'd bet a CI check could be added to check that if the opcodes change then Python/importlib_external.h changes.

    @dpgeorge
    Copy link
    Mannequin

    dpgeorge mannequin commented Oct 21, 2021

    It looks like this change introduced a subtle, and maybe intended (?), behavioural change.

    Consider (from MicroPython's test suite):

    def f():
        n = 0 
        while True:
            n = yield n + 1 
            print(n)
    
    g = f()
    try:
        g.send(1)
    except TypeError:
        print("caught")
    
    print(g.send(None))
    print(g.send(100))
    print(g.send(200))

    This used to work prior to commit b37181e. But after that commit it fails on the print(g.send(None)) because the generator is now stopped.

    @markshannon
    Copy link
    Member Author

    Damien, thanks for catching this.

    The change was not intended.

    There are two kind of exceptions raised by send.

    1. Where a pre-condition is not met, e.g. a generator is already ruuning (caller errors)
    2. When the generator/coroutine raises an exception (callee exceptions).

    (1) does not change the state of the generator, (2) does.

    Sending non-None to a generator that has not started falls into kind 1, IMO. So we should revert the change.

    @markshannon
    Copy link
    Member Author

    Just to be clear, it is the behavior change that should be reverted, not necessarily the new bytecode.

    In fact we should probably push on with (2) and add an exception handler wrapping the whole generator except the GEN_START. That way a GEN_START exception will not clear the generator.
    GEN_START might need to do a bit of trickery like setting lasti back to -1.

    @dpgeorge
    Copy link
    Mannequin

    dpgeorge mannequin commented Oct 27, 2021

    Thanks for confirming the bug.

    Sending non-None to a not-started generator could arguably be case (2), because that's exactly the semantics introduced by the commit that broke the test case :)

    Honestly I don't have a strong opinion on which way this goes. But I think it would be interesting to know if there was code out there that relied on the original behaviour.

    @pablogsal
    Copy link
    Member

    Mark, is something left in this issue?

    @sweeneyde
    Copy link
    Member

    bpo-46009 about the same behavior change

    @pablogsal
    Copy link
    Member

    Unfortunately, this has not been fixed into 3.10.1 and 3.11.0a3 as this hasn't version information and therefore has missed our automatic checks for blockers.

    I have marked https://bugs.python.org/issue46009? as release blocker, as well as this issue and I have marked the versions.

    Please, ensure this is fixed ASAP so it doesn't go into the next bugfix release or alpha release.

    @pablogsal pablogsal added 3.10 only security fixes 3.11 only security fixes labels Dec 7, 2021
    @markshannon
    Copy link
    Member Author

    Sorry about the delay; this dropped off my list. I'll fix it shortly.

    This is quite an obscure bug, so I'm not sure that it is worth blocking the release for. But I'm not the release manager :)

    @pablogsal
    Copy link
    Member

    This is quite an obscure bug, so I'm not sure that it is worth blocking the release for. But I'm not the release manager :)

    Well, it certainly didn't block 3.10.1 or 3.11.0a3 ;)

    @pablogsal
    Copy link
    Member

    Is there anything left here?

    @markshannon
    Copy link
    Member Author

    Yes, most of it :)

    We haven't implemented points 2 and 3, yet.

    I'm in no hurry to implement 3. It would clean up gen.throw a lot, and break the dependency between that code and the interpreter, but it isn't urgent.

    2 is more urgent. I think we need it to allow specialization of FOR_ITER for generators, so it should happen in the next few weeks.

    @markshannon
    Copy link
    Member Author

    New changeset 0367a36 by Mark Shannon in branch 'main':
    bpo-43683: Streamline YIELD_VALUE and SEND (GH-30723)
    0367a36

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel
    Copy link
    Member

    CC @brandtbucher .

    @iritkatriel iritkatriel added the 3.12 bugs and security fixes label Sep 13, 2022
    carljm added a commit to carljm/cpython that referenced this issue Feb 13, 2023
    * main:
      pythongh-101810: Remove duplicated st_ino calculation (pythonGH-101811)
      pythongh-92547: Purge sqlite3_enable_shared_cache() detection from configure (python#101873)
      pythonGH-100987: Refactor `_PyInterpreterFrame` a bit, to assist generator improvement. (pythonGH-100988)
      pythonGH-87849: Simplify stack effect of SEND and specialize it for generators and coroutines. (pythonGH-101788)
      Correct trivial grammar in reset_mock docs (python#101861)
      pythongh-101845: pyspecific: Fix i18n for availability directive (pythonGH-101846)
      pythongh-89792: Limit test_tools freeze test build parallelism based on the number of cores (python#101841)
      pythongh-85984: Utilize new "winsize" functions from termios in pty tests. (python#101831)
      pythongh-89792: Prevent test_tools from copying 1000M of "source" in freeze test (python#101837)
      Fix typo in test_fstring.py (python#101823)
      pythonGH-101797: allocate `PyExpat_CAPI` capsule on heap (python#101798)
      pythongh-101390: Fix docs for `imporlib.util.LazyLoader.factory` to properly call it a class method (pythonGH-101391)
    markshannon added a commit that referenced this issue Feb 15, 2023
    carljm added a commit to carljm/cpython that referenced this issue May 7, 2023
    carljm added a commit to carljm/cpython that referenced this issue May 11, 2023
    * main: (27 commits)
      pythongh-87849: fix SEND specialization family definition (pythonGH-104268)
      pythongh-101819: Adapt _io.IOBase.seek and _io.IOBase.truncate to Argument Clinic (python#104384)
      pythongh-101819: Adapt _io._Buffered* methods to Argument Clinic (python#104367)
      pythongh-101819: Refactor `_io` futher in preparation for module isolation (python#104369)
      pythongh-101819: Adapt _io.TextIOBase methods to Argument Clinic (python#104383)
      pythongh-101117: Improve accuracy of sqlite3.Cursor.rowcount docs (python#104287)
      pythonGH-92184: Convert os.altsep to '/' in filenames when creating ZipInfo objects (python#92185)
      pythongh-104357: fix inlined comprehensions that close over iteration var (python#104368)
      pythonGH-90208: Suppress OSError exceptions from `pathlib.Path.glob()` (pythonGH-104141)
      pythonGH-102181: Improve specialization stats for SEND (pythonGH-102182)
      pythongh-103000: Optimise `dataclasses.asdict` for the common case (python#104364)
      pythongh-103538: Remove unused TK_AQUA code (pythonGH-103539)
      pythonGH-87695: Fix OSError from `pathlib.Path.glob()` (pythonGH-104292)
      pythongh-104263: Rely on Py_NAN and introduce Py_INFINITY (pythonGH-104202)
      pythongh-104010: Separate and improve docs for `typing.get_origin` and `typing.get_args` (python#104013)
      pythongh-101819: Adapt _io._BufferedIOBase_Type methods to Argument Clinic (python#104355)
      pythongh-103960: Dark mode: invert image brightness (python#103983)
      pythongh-104252: Immortalize Py_EMPTY_KEYS (pythongh-104253)
      pythongh-101819: Clean up _io windows console io after pythongh-104197 (python#104354)
      pythongh-101819: Harden _io init (python#104352)
      ...
    carljm added a commit to carljm/cpython that referenced this issue May 11, 2023
    * main:
      pythongh-87849: fix SEND specialization family definition (pythonGH-104268)
      pythongh-101819: Adapt _io.IOBase.seek and _io.IOBase.truncate to Argument Clinic (python#104384)
      pythongh-101819: Adapt _io._Buffered* methods to Argument Clinic (python#104367)
      pythongh-101819: Refactor `_io` futher in preparation for module isolation (python#104369)
      pythongh-101819: Adapt _io.TextIOBase methods to Argument Clinic (python#104383)
      pythongh-101117: Improve accuracy of sqlite3.Cursor.rowcount docs (python#104287)
      pythonGH-92184: Convert os.altsep to '/' in filenames when creating ZipInfo objects (python#92185)
      pythongh-104357: fix inlined comprehensions that close over iteration var (python#104368)
      pythonGH-90208: Suppress OSError exceptions from `pathlib.Path.glob()` (pythonGH-104141)
    @gvanrossum
    Copy link
    Member

    @markshannon Is this done?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes 3.11 only security fixes 3.12 bugs and security fixes performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants