Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

GH-90997: Wrapyield from/await in a virtualtry/except StopIteration#96010

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
brandtbucher merged 13 commits intopython:mainfrombrandtbucher:gen-throw-new
Aug 19, 2022

Conversation

brandtbucher
Copy link
Member

@brandtbucherbrandtbucher commentedAug 15, 2022
edited
Loading

Supersedes#31968 by setting up a virtualtry/except StopIteration to handle the case where a delegated sub-generator returns a value during aclose orthrow call through a suspended frame. This moves the logic out of the generatorthrow implementation and into the bytecode, which requires fewer frame hacks.It does, however, require replacingLOAD_ASSERTION_ERROR with a more generalLOAD_EXCEPTION_TYPE opcode that can also handleStopIteration.

It also updates the docs forSEND, which are incomplete.

1: c
0: value
1: b
2: c
Variable names:
0: a
1: d"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It would be good to also add a test that shows what you're fixing here.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hm, we're not really fixing anything (in fact, we want the behavior to stay exactly the same). All we're really doing is moving some of the error handling logic forclose andthrow into the bytecode, so I'm not really sure how to test that.

I'm confident that the tests intest_yield_from.TestInterestingEdgeCases exercise this path enough to prevent any behavior changes (like we've seen with other approaches to address this issue).

@markshannon
Copy link
Member

This adds a lot of bulk to the bytecode.
Take this minimal function:

asyncdeffoo(x):awaitx

main

  1           0 RETURN_GENERATOR              2 POP_TOP              4 RESUME                   0  2           6 LOAD_FAST                0 (x)              8 GET_AWAITABLE            0             10 LOAD_CONST               0 (None)        >>   12 SEND                     3 (to 20)             14 YIELD_VALUE              2             16 RESUME                   3             18 JUMP_BACKWARD_NO_INTERRUPT     4 (to 12)        >>   20 POP_TOP             22 LOAD_CONST               0 (None)             24 RETURN_VALUE

this PR

  1           0 RETURN_GENERATOR              2 POP_TOP              4 RESUME                   0  2           6 LOAD_FAST                0 (x)              8 GET_AWAITABLE            0             10 LOAD_CONST               0 (None)        >>   12 SEND                     3 (to 20)             14 YIELD_VALUE              2             16 RESUME                   3             18 JUMP_BACKWARD_NO_INTERRUPT     4 (to 12)        >>   20 POP_TOP             22 LOAD_CONST               0 (None)             24 RETURN_VALUE        >>   26 LOAD_EXCEPTION_TYPE      1 (StopIteration)             28 CHECK_EXC_MATCH             30 POP_JUMP_FORWARD_IF_TRUE     1 (to 34)             32 RERAISE                  0        >>   34 LOAD_ATTR                0 (value)             54 SWAP                     3             56 POP_TOP             58 POP_TOP             60 POP_TOP             62 LOAD_CONST               0 (None)             64 RETURN_VALUEExceptionTable:  14 to 14 -> 26 [2]

What I had envisioned

  1           0 RETURN_GENERATOR              2 POP_TOP              4 RESUME                   0  2           6 LOAD_FAST                0 (x)              8 GET_AWAITABLE            0             10 LOAD_CONST               0 (None)        >>   12 SEND                     3 (to 20)             14 YIELD_VALUE              2             16 RESUME                   3             18 JUMP_BACKWARD_NO_INTERRUPT     4 (to 12)        >>   20 POP_TOP             22 LOAD_CONST               0 (None)             24 RETURN_VALUE             26 THROW                (to 20)             28 JUMP_BACKWARD_NO_INTERRUPT     (to 14)ExceptionTable:  14 to 14 -> 26 [2]

WhereTHROW throws into the delegated awaitable.
If that delegated awaitable returns, then jump.
if it yields, then push the value.
If it raises, then propagate.

Much likeSEND sends to the delegated awaitable,THROW throws into it.

See alsofaster-cpython/ideas#448

@brandtbucher
Copy link
MemberAuthor

brandtbucher commentedAug 17, 2022
edited
Loading

This adds a lot of bulk to the bytecode.

Yeah, like I said in the meetings: this about triples the size of the bytecode (all in the cold path, though). If all we care about here is the literal size of the bytecode, then we can pack everything in the exception handler into its own dedicated instruction.

This would look a lot likeEND_ASYNC_FOR, except it would push a value and jump on return. But combining the instructions like that seems to be moving in the wrong direction - in fact, I'd like to seeEND_ASYNC_FOR broken up like we have here.

What I had envisioned

  1           0 RETURN_GENERATOR              2 POP_TOP              4 RESUME                   0  2           6 LOAD_FAST                0 (x)              8 GET_AWAITABLE            0             10 LOAD_CONST               0 (None)        >>   12 SEND                     3 (to 20)             14 YIELD_VALUE              2             16 RESUME                   3             18 JUMP_BACKWARD_NO_INTERRUPT     4 (to 12)        >>   20 POP_TOP             22 LOAD_CONST               0 (None)             24 RETURN_VALUE             26 THROW                (to 20)             28 JUMP_BACKWARD_NO_INTERRUPT     (to 14)ExceptionTable:  14 to 14 -> 26 [2]

WhereTHROW throws into the delegated awaitable. If that delegated awaitable returns, then jump. if it yields, then push the value. If it raises, then propagate.

Much likeSEND sends to the delegated awaitable,THROW throws into it.

See alsofaster-cpython/ideas#448

I've been trying to get something like this to work for the last few weeks in between release blocker firefighting and vacations, but I've just been left convinced that there are too many wrinkles to make it this elegant. Even setting aside the relatively minor annoyance that we need twoTHROW instructions right now (one forward and one backward), here are just a couple of the issues make this much more complicated than "justSEND but callthrow":

  • We actually need to callclose if the exception is aGeneratorExit...
  • ...except if we're getting it from anathrow oraclose call, in which case we have different logic for deciding to callathrow oraclose, which have different semantics in theGeneratorExit case. Grep forclose_on_genexit ingenobject.c for the first breadcrumb in the trail of special async generator logic.
  • We aren't supposed to chain a thrown/closingGeneratorExit to theRuntimeError complaining that we've ignored it, which is a pain to do without changing how literally all other exceptions are chained.
  • The current code has fast paths for closing or throwing into a delegated generator or coroutine if they are exact. Moving all throws and closes into the bytecode would probably make the majority of throws and closes (including generator finalization) a bit slower. If we want to leave these fast paths in the C code, then I don't much see the point of moving the rest intoTHROW.

For reference, here is mymostly-but-not-quite workingTHROW branch. If we're really sure that it's worth trying to hack around these issues (and others) in theTHROW instructions, then I can keep plugging away at it. But this PR only extracts the problematic parts of thethrow/close implementation while leaving all of the other subtle logic alone. That feels like the right separation between the C/Python boundary, in my opinion.

Add to all this that I'm very confident that this PR behaves correctly, and I understand it very well, but am very far from being able to promise that aTHROW implementation will preserve the exact semantics for things like async generators and coroutines, of which I know very, very little.

@brandtbucher
Copy link
MemberAuthor

brandtbucher commentedAug 17, 2022
edited
Loading

Also note that for many compiler-generatedawait loops, we can simplify the emitted bytecode to skip extracting and pushing the return value where the compiler knows it's not used. Doing the same for allawait andyield from expressions would be possible but harder, since by the point we're emitting it we're buried deep in general-purpose expression-compiling code and don't have much context on how the result is used.

@markshannon
Copy link
Member

markshannon commentedAug 17, 2022
edited
Loading

Sure, there are many complications around what is thrown, or whether close is called, etc.
First, move all those complications into a new function,_Py_throw_into().

With this new function:

PySendResult_Py_throw_into(PyGenObject*subgen,intclose_on_genexit,PyObject*typ,PyObject*val,PyObject*tb,PyObject**presult);

We can initially refactor_gen_throw to:

staticPyObject*_gen_throw(PyGenObject*gen,intclose_on_genexit,PyObject*typ,PyObject*val,PyObject*tb){PyObject*yf=_PyGen_yf(gen);if (yf) {PyObect*res;PySendResultwhat=_Py_throw_into(yf,close_on_genexit,type,val,tb,&res);switch(what) {casePYGEN_RETURN:/* Stop SEND loop *//* Awkward popping and jumping goes here */returngen_send(gen,val);casePYGEN_ERROR:returnNULL;casePYGEN_NEXT:returnres;        }    }throw_here:    ...

That doesn't help much by itself, but_Py_throw_into is what we need to implementTHROW which, as you point out, cannot jump because it needsclose_on_genexit as its oparg.

Instead of generating

  THROW  target  JUMP_BACKWARD_NO_INTERRUPT back_to_yield

we generate

  THROW close_on_genexit  POP_JUMP_IF_TRUE target  JUMP_BACKWARD_NO_INTERRUPT back_to_yield

THROW should be little more than a wrapper around_Py_throw_into, raising onPYGEN_ERROR, pushingTrue forPYGEN_RETURN andFalse forPYGEN_NEXT.

@markshannon
Copy link
Member

See also#96040

@brandtbucher
Copy link
MemberAuthor

Closing and reopening to re-run the Azure Pipelines failure.

@brandtbucher
Copy link
MemberAuthor

@markshannon, I've updated the PR based on our discussion this morning. Let me know how it looks.

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a definite improvement over the current implementation.

Ultimately we want all the handling of sub-generators to done in bytecode.
This is a good first step, moving the flow control into the bytecode.

@@ -567,6 +567,17 @@ the original TOS1.
.. versionchanged:: 3.11
Exception representation on the stack now consist of one, not three, items.


.. opcode:: END_THROW
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

END suggests that this is the end of a sequence of bytecodes.
MaybeCLEANUP_THROW as it cleans up_gen_throw?

brandtbucher reacted with thumbs up emoji
// Set up a virtual try/except to handle when StopIteration is raised during
// a close or throw call. The only way YIELD_VALUE raises if they do!
ADDOP_JUMP(c, SETUP_FINALLY, fail);
RETURN_IF_FALSE(compiler_push_fblock(c, TRY_EXCEPT, send, NO_LABEL, NULL));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There can be no call tocompiler_unwind_fblock before this block is popped, so we don't need to push a block here.

brandtbucher reacted with thumbs up emoji
ADDOP_I(c, YIELD_VALUE, 0);
compiler_pop_fblock(c, TRY_EXCEPT, send);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nor pop it here

brandtbucher reacted with thumbs up emoji
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment:I have made the requested changes; please review again.

@brandtbucher
Copy link
MemberAuthor

This is a definite improvement over the current implementation.

Ultimately we want all the handling of sub-generators to done in bytecode. This is a good first step, moving the flow control into the bytecode.

Yeah, this fixes the main issue we were concerned with, but I agree that it makes sense to keep trying to port over more of thethrow logic. I plan to return to this whenever I get a few extra cycles.

@brandtbucherbrandtbucher merged commit5bfb3c3 intopython:mainAug 19, 2022
@encukou
Copy link
Member

With this PR, async functions with exactly 20 nested blocks started crashing with anassert failure, e.g.:

async def t():    async with h,t,t,o,f,y,o,t,r,o,f,t,f,r,t,m,r,o,t,l:n
python: Python/compile.c:7185: pop_except_block: Assertion `stack->depth > 0' failed.

see#116767

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@iritkatrieliritkatrieliritkatriel left review comments

@markshannonmarkshannonmarkshannon requested changes

@1st11st1Awaiting requested review from 1st1

Assignees

@brandtbucherbrandtbucher

Labels
interpreter-core(Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@brandtbucher@markshannon@bedevere-bot@encukou@iritkatriel

[8]ページ先頭

©2009-2025 Movatter.jp