Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
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
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| 2: c | ||
| Variable names: | ||
| 0: a | ||
| 1: d""" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 commentedAug 16, 2022
This adds a lot of bulk to the bytecode. asyncdeffoo(x):awaitx mainthis PRWhat I had envisionedWhere Much like See alsofaster-cpython/ideas#448 |
brandtbucher commentedAug 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 like
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 two
For reference, here is mymostly-but-not-quite working 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 a |
brandtbucher commentedAug 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Also note that for many compiler-generated |
markshannon commentedAug 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sure, there are many complications around what is thrown, or whether close is called, etc. 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 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 Instead of generating we generate
|
markshannon commentedAug 17, 2022
See also#96040 |
brandtbucher commentedAug 17, 2022
Closing and reopening to re-run the Azure Pipelines failure. |
brandtbucher commentedAug 17, 2022
@markshannon, I've updated the PR based on our discussion this morning. Let me know how it looks. |
markshannon left a comment
There was a problem hiding this 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.
Doc/library/dis.rst Outdated
| Exception representation on the stack now consist of one, not three, items. | ||
| ..opcode::END_THROW |
There was a problem hiding this comment.
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?
Python/compile.c Outdated
| // 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)); |
There was a problem hiding this comment.
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.
Python/compile.c Outdated
| ADDOP_JUMP(c,SETUP_FINALLY,fail); | ||
| RETURN_IF_FALSE(compiler_push_fblock(c,TRY_EXCEPT,send,NO_LABEL,NULL)); | ||
| ADDOP_I(c,YIELD_VALUE,0); | ||
| compiler_pop_fblock(c,TRY_EXCEPT,send); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nor pop it here
bedevere-bot commentedAug 19, 2022
When you're done making the requested changes, leave the comment: |
brandtbucher commentedAug 19, 2022
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 the |
encukou commentedApr 8, 2024
With this PR, async functions with exactly 20 nested blocks started crashing with an see#116767 |
Uh oh!
There was an error while loading.Please reload this page.
Supersedes#31968 by setting up a virtual
try/except StopIterationto handle the case where a delegated sub-generator returns a value during acloseorthrowcall through a suspended frame. This moves the logic out of the generatorthrowimplementation and into the bytecode, which requires fewer frame hacks.It does, however, require replacingLOAD_ASSERTION_ERRORwith a more generalLOAD_EXCEPTION_TYPEopcode that can also handleStopIteration.It also updates the docs for
SEND, which are incomplete.