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

GH-96421: Insert shim frame on entry to interpreter #96319

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Aug 26, 2022

Simplifies RETURN_VALUE, RETURN_GENERATOR and YIELD_VALUE as they no longer need to check if the current frame is the entry frame.

Should allow specialization of FOR_ITER and SEND for generators and coroutines.

Needs docs and news.

Performance impact is about zero

@markshannon markshannon changed the title Insert shim frame on entry to interpreter GH-96421: Insert shim frame on entry to interpreter Aug 30, 2022
@sweeneyde
Copy link
Member

sweeneyde commented Sep 8, 2022

Does this add much extra CPU/stack/memory overhead for calling Python from C? Or alternating py/c/py/c/py calls? Probably not the most important use-cases, but I would hope that they wouldn't pessimize too much.

@markshannon
Copy link
Member Author

markshannon commented Sep 9, 2022

Does this add much extra CPU/stack/memory overhead for calling Python from C?

  • CPU: A little, but it speeds up the return sequence. The net effect is about zero
  • C Stack consumption is increased, but only 80 bytes per call.

Python/pylifecycle.c Show resolved Hide resolved
@markshannon markshannon marked this pull request as ready for review Oct 19, 2022
@markshannon
Copy link
Member Author

markshannon commented Oct 20, 2022

The performance impact of this negative but I want to merge anyway because:

Copy link
Member

@brandtbucher brandtbucher left a comment

Thanks for taking the time to do this. There's definitely a lot of subtlety involved.

In general, I think that this adds a fair amount of complexity that wasn't present before (I've recently gained new appreciation for the value of being able to reason about the web of frames that exist in 3.11+, particularly things like visibility and ownership rules). If you're confident that we can recoup the perf cost and complexity with work that builds upon this, though, I think it makes sense as a first step towards that.

Also, I think that the growing set of rules around things like "ownership", "completeness", and "state" are becoming a real maintenance burden. We might consider tightening up those concepts in 3.12, because there are lots of complex relationships and invalid states that are crucial to understand when working on (or with) interpreter internals. I'm thinking not only about us, but also third-party libraries like Greenlet and forks like Cinder.

Lib/opcode.py Show resolved Hide resolved
Objects/frameobject.c Outdated Show resolved Hide resolved
Python/pylifecycle.c Show resolved Hide resolved
Python/traceback.c Show resolved Hide resolved
Objects/codeobject.c Outdated Show resolved Hide resolved
Objects/codeobject.c Outdated Show resolved Hide resolved
Python/pylifecycle.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Oct 20, 2022

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

@markshannon
Copy link
Member Author

markshannon commented Oct 21, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Oct 21, 2022

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from brandtbucher Oct 21, 2022
@brandtbucher
Copy link
Member

brandtbucher commented Oct 21, 2022

I don't know if I'll have time to review again today (still need to prepare my talk for the release stream). Is this able to wait until next week? If not, I can try to make time.

@markshannon
Copy link
Member Author

markshannon commented Oct 21, 2022

It can wait until next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants