Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-46409: Make generators in bytecode#30633
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
markshannon commented Jan 17, 2022 • edited by gvanrossum
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by gvanrossum
Uh oh!
There was an error while loading. Please reload this page.
markshannon commented Jan 17, 2022
Fixes https://bugs.python.org/issue46389 and https://bugs.python.org/issue46374, but does not include tests for those issues. |
bedevere-bot commented Jan 17, 2022
🤖 New build scheduled with the buildbot fleet by @markshannon for commit c33abbf 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot commented Jan 18, 2022
🤖 New build scheduled with the buildbot fleet by @markshannon for commit 243e441 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
gvanrossum 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.
I'm still trying to follow the rest of the C code here, but in case I fail, here's the rest of my review. :-)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @@ -0,0 +1,2 @@ | |||
| Add new bytecode to make generators. Simplifies calling Python functions in | |||
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.
I'd name the new bytecode(s) explicitly here.
Uh oh!
There was an error while loading. Please reload this page.
| and coroutine objects. */ | ||
| #define_PyGenObject_HEAD(prefix) \ | ||
| PyObject_HEAD \ | ||
| /* Note: gi_frame can be NULL if the generator is "finished" */ \ |
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.
I have a question about gi_iframe. Below (L31) it is defined as an array of length 1 of object pointers. But everywhere it's used, it is cast to InterpreterFrame *. That's not an object or object pointer AFAICT. So what's going on here? Is the declaration wrong? An intentional lie?
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 is a lie.
We don't want to expose InterpreterFrame in public headers, so we can't do the sensible thing and declare gi_frame as:
InterpreterFramegi_frame;as C won't allow incomplete types in structs, even as the last member.
We could improve this by breaking up the header, so the public API sees a different definition. Still a lie, but a more elegant one.
Public header:
typedefstruct{_PyGenObject_HEAD(gi) } PyGenObject;Private header:
typedefstruct{_PyGenObject_HEAD(gi) InterpreterFramegi_frame} PyGenObject;Probably best done in a different PR, though.
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.
Gotcha. Any type other than PyObject* would be better though :-). And if you replace the casts with a macro it’s easier to fix later. I have an idea for the macro but it’s too painful to type on a phone.
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.
The first field of InterpreterFrame is PyFunctionObject * and the first few fields are all PyObject *, so declaring gi_frame as PyObject * gi_frame[1] seemed safe. Do let me know what your macro is, once you at a PC.
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.
Do let me know what your macro is, once you at a PC.
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.
Uh oh!
There was an error while loading. Please reload this page.
gvanrossum commented Jan 19, 2022
PS. What do you mean by "coroutines are stackful"? (I was hoping that reading the code would clarify this, but either I didn't get to that point in the code yet or it's hidden by details.) |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
markshannon commented Jan 19, 2022
Stackful coroutines have their own stack, like Lua coroutines or greenlets. |
e31703c to 17b453aCompare
Adds a
RETURN_GENERATORbytecode which makes a generator (or coroutine) from the current frame.This has a number of advantages:
This PR also adds a
JUMP_NO_INTERRUPTbytecode. This is the same asJUMP_ABSOLUTEwithout any checks for interrupts. Checking for interrupts inyield fromwould destroy the flaky pretense that Python coroutines are stackful. (They are not, but PEPs 380 and 489 like to pretend that they are), so we need the extra, seemingly redundant instruction.We use
RETURN_GENERATORrather thanMAKE_GENERATOR; RETURN_VALUEas theRETURN_VALUEwould make the compiler treat all the following code as unreachable. We could fix that using artificial instructions, but I think that is best left to another PR.https://bugs.python.org/issue46409