GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-96421: Insert shim frame on entry to interpreter by markshannon · Pull Request #96319 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Aug 26, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • Issue: Insert shim frames at entries points to the interpreter. #96421

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
markshannon added 3 commits August 24, 2022 11:26
@markshannon
Handle _PyEval_EvalFrameDefault exit in bytecode.
4d5e9a5
@markshannon
Merge branch 'main' into entry-frame
cf78d2b
@markshannon
Get entry frame shim working.
7f5348a
@bedevere-bot bedevere-bot added the awaiting core review label Aug 26, 2022
@markshannon
markshannon requested a review from brandtbucher August 26, 2022 17:46
@markshannon
Delay destruction of trampoline code object and make symbol private
59e7727
@markshannon markshannon changed the title Insert shim frame on entry to interpreter GH-96421: Insert shim frame on entry to interpreter Aug 30, 2022
markshannon added 3 commits August 30, 2022 16:20
@markshannon
Make sure that previous never points into the C stack after return.
9f710e6
@markshannon
Use explicit stacksize in _Py_MakeTrampoline. Allow interpreter to cl…
4ee84ab 
…ear previous frame pointer.
@markshannon
Move assert before use.
f5161e3
@markshannon
markshannon force-pushed the entry-frame branch from e62e658 to 551a66e Compare August 31, 2022 11:35
@markshannon
Use typedef name.
06c2ad6
@markshannon
markshannon force-pushed the entry-frame branch from 551a66e to 06c2ad6 Compare August 31, 2022 11:36
markshannon added 4 commits September 5, 2022 10:02
@markshannon
Merge branch 'main' into entry-frame
cb6134e
@markshannon
Merge branch 'main' into entry-frame
11b3b17
@markshannon
More on-stack C structs into a big struct and add sentinels, as we wa…
053f488 
…nt safety checks but ASAN gets tripped up in some cases.
@markshannon
Skip a test if ASAN is turned on.
4dad69a
@markshannon markshannon mentioned this pull request Sep 8, 2022
Specialization of generator iteration and coroutine await faster-cpython/ideas#457
Closed
@sweeneyde

sweeneyde commented Sep 8, 2022

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon added 3 commits September 9, 2022 10:31
@markshannon
Remove extra semicolons.
0db3b25
@markshannon
Add news
05b4f68
@markshannon
Merge branch 'main' into entry-frame
6cca61c
@markshannon

markshannon commented Sep 9, 2022

Copy link
Copy Markdown
Member Author

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.
sweeneyde reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde
sweeneyde reviewed Sep 11, 2022
View reviewed changes
Comment thread Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
Merge branch 'main' into entry-frame
5099861
@markshannon markshannon mentioned this pull request Sep 13, 2022
Enable specialization of iteration over generators, for gen():, and awaiting coroutines, await coro() #96793
Closed
4 tasks done
markshannon added 3 commits September 14, 2022 16:24
@markshannon
Halve the number of writes for shim frame.
4015c72
@markshannon
Remove one more write to shim frame.
fb19e94
@markshannon
Fix lltrace
95ef81b
@markshannon markshannon mentioned this pull request Sep 15, 2022
GH-96421: Don't keep local copies of first_instr, names and consts in the interpreter. #96847
Closed
@bedevere-bot
bedevere-bot requested a review from brandtbucher October 21, 2022 16:57
@brandtbucher

brandtbucher commented Oct 21, 2022

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon

markshannon commented Oct 21, 2022

Copy link
Copy Markdown
Member Author

It can wait until next week
brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brandtbucher brandtbucher mentioned this pull request Oct 28, 2022
GH-96421: Introduce cleanup contexts for inlined frames #98795
Closed
brandtbucher
brandtbucher reviewed Oct 31, 2022
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

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.

Mostly nits on the changes.

Also, I'd like to discuss this vs. something like #98795 again at our stand-up, if that's okay. This still feels pretty heavy/breaky to me compared to that PR (and I'm still not clear what it actually gives us over that approach).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-10-19-15-59-08.gh-issue-96421.e22y3r.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@markshannon markshannon mentioned this pull request Oct 31, 2022
GH-96793: Specialize FOR_ITER for generators. #98772
Merged
gvanrossum
gvanrossum reviewed Nov 3, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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 had some earlier comments pending (not sure if they still apply). But mainly this is a reminder that instructions are now in bytecodes.c, and changes there require running make regen-cases.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_subprocess.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/genobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
markshannon added 3 commits November 7, 2022 16:04
@markshannon
Address review comments
bd29356
@markshannon
Merge branch 'main' into entry-frame
9446325
@markshannon
Post merge cleanup
f6a6457
@bedevere-bot bedevere-bot mentioned this pull request Nov 8, 2022
Insert shim frames at entries points to the interpreter. #96421
Closed
@markshannon
Turn ASAN back on for subprocess test.
d113655
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 8, 2022
@bedevere-bot

bedevere-bot commented Nov 8, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @markshannon for commit d113655 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 8, 2022
brandtbucher
brandtbucher approved these changes Nov 9, 2022
View reviewed changes

@brandtbucher brandtbucher left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Just a couple of remaining nits.

I'm not sure what kind of review you're looking for... I don't see any technical issues with the implementation, so I'll hit "approve". :)

But I still have reservations about this change being much deeper (and probably more expensive) than what's really necessary right now. I still feel like I haven't really gotten an answer on why this change is more appropriate than something like #98795. So I don't necessarily approve of this approach, but I also won't block you from merging this if you still feel differently.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_subprocess.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-10-19-15-59-08.gh-issue-96421.e22y3r.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/opcode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Nov 9, 2022
markshannon and others added 3 commits November 9, 2022 16:39
@markshannon @brandtbucher
Apply suggestions from code review
ed7af1e 
Co-authored-by: Brandt Bucher <[email protected]>
@markshannon
Merge branch 'main' into entry-frame
48410d9
@markshannon
Merge branch 'main' into entry-frame
7cda3ef
@markshannon
markshannon merged commit 1e197e6 into python:main Nov 10, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 10, 2022
@markshannon
markshannon deleted the entry-frame branch November 10, 2022 13:34
ethanfurman pushed a commit to ethanfurman/cpython that referenced this pull request Nov 12, 2022
@markshannon @ethanfurman
pythonGH-96421: Insert shim frame on entry to interpreter (pythonGH-9…
e0562ab 
…6319)

* Adds EXIT_INTERPRETER instruction to exit PyEval_EvalDefault()

* Simplifies RETURN_VALUE, YIELD_VALUE and RETURN_GENERATOR instructions as they no longer need to check for entry frames.
@P403n1x87 P403n1x87 mentioned this pull request Aug 18, 2023
GH-108035: Remove the _PyCFrame struct as it is no longer needed for performance. #108036
Merged
This was referenced Dec 12, 2023
Can't access full stack of suspended greenlet on 3.12 python-greenlet/greenlet#388
Closed
FRAME_OWNED_BY_CSTACK breaks greenlet introspection; can CPython help? #113037
Open
@jiridanek jiridanek mentioned this pull request Jan 2, 2024
Possible heap-use-after-free in ctypes in Python 3.12 #113576
Closed
@user202729 user202729 mentioned this pull request Feb 4, 2025
gh-129280: Avoid cyclic reference in contextlib.contextmanager #129276
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments
@sweeneyde sweeneyde sweeneyde left review comments
@brandtbucher brandtbucher brandtbucher approved these changes
@gpshead gpshead Awaiting requested review from gpshead
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@markshannon @sweeneyde @bedevere-bot @brandtbucher @gvanrossum
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.