GH-96421: Insert shim frame on entry to interpreter by markshannon · Pull Request #96319 · python/cpython · GitHub

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

…ear previous frame pointer.

…nt safety checks but ASAN gets tripped up in some cases.

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.

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.

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.

It can wait until next week

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).

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.

🤖 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.

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.

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

…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.

markshannon added 3 commits August 24, 2022 11:26

Handle _PyEval_EvalFrameDefault exit in bytecode.

4d5e9a5

Merge branch 'main' into entry-frame

cf78d2b

Get entry frame shim working.

7f5348a

bedevere-bot added the awaiting core review label Aug 26, 2022

markshannon requested a review from brandtbucher August 26, 2022 17:46

Delay destruction of trampoline code object and make symbol private

59e7727

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

Make sure that previous never points into the C stack after return.

9f710e6

Use explicit stacksize in _Py_MakeTrampoline. Allow interpreter to cl…

4ee84ab

…ear previous frame pointer.

Move assert before use.

f5161e3

markshannon force-pushed the entry-frame branch from e62e658 to 551a66e Compare August 31, 2022 11:35

Use typedef name.

06c2ad6

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

Merge branch 'main' into entry-frame

cb6134e

Merge branch 'main' into entry-frame

11b3b17

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.

Skip a test if ASAN is turned on.

4dad69a

markshannon mentioned this pull request Sep 8, 2022

Specialization of generator iteration and coroutine await faster-cpython/ideas#457

Closed

markshannon added 3 commits September 9, 2022 10:31

Remove extra semicolons.

0db3b25

Add news

05b4f68

Merge branch 'main' into entry-frame

6cca61c

sweeneyde reviewed Sep 11, 2022

View reviewed changes

Comment thread Python/pylifecycle.c

Merge branch 'main' into entry-frame

5099861

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

markshannon added 3 commits September 14, 2022 16:24

Halve the number of writes for shim frame.

4015c72

Remove one more write to shim frame.

fb19e94

Fix lltrace

95ef81b

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 requested a review from brandtbucher October 21, 2022 16:57

brandtbucher mentioned this pull request Oct 28, 2022

GH-96421: Introduce cleanup contexts for inlined frames #98795

Closed

brandtbucher reviewed Oct 31, 2022

View reviewed changes

markshannon mentioned this pull request Oct 31, 2022

GH-96793: Specialize FOR_ITER for generators. #98772

Merged

gvanrossum reviewed Nov 3, 2022

View reviewed changes

markshannon added 3 commits November 7, 2022 16:04

Address review comments

bd29356

Merge branch 'main' into entry-frame

9446325

Post merge cleanup

f6a6457

bedevere-bot mentioned this pull request Nov 8, 2022

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

Closed

Turn ASAN back on for subprocess test.

d113655

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 8, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 8, 2022

brandtbucher approved these changes Nov 9, 2022

View reviewed changes

Comment thread Lib/test/test_subprocess.py Outdated

Comment thread Misc/NEWS.d/next/Core and Builtins/2022-10-19-15-59-08.gh-issue-96421.e22y3r.rst Outdated

Comment thread Lib/opcode.py

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

Apply suggestions from code review

ed7af1e

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

Merge branch 'main' into entry-frame

48410d9

Merge branch 'main' into entry-frame

7cda3ef

markshannon merged commit 1e197e6 into python:main Nov 10, 2022

bedevere-bot removed the awaiting merge label Nov 10, 2022

markshannon deleted the entry-frame branch November 10, 2022 13:34

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 mentioned this pull request Jan 2, 2024

Possible heap-use-after-free in ctypes in Python 3.12 #113576

Closed

user202729 mentioned this pull request Feb 4, 2025

gh-129280: Avoid cyclic reference in contextlib.contextmanager #129276

Draft

GH-96421: Insert shim frame on entry to interpreter by markshannon · Pull Request #96319 · python/cpython · GitHub

Uh oh!

Uh oh!

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

GH-96421: Insert shim frame on entry to interpreter#96319markshannon merged 39 commits intopython:mainpython/cpython:mainfrom faster-cpython:entry-framefaster-cpython/cpython:entry-frameCopy head branch name to clipboard

Conversation

markshannon commented Aug 26, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sweeneyde commented Sep 8, 2022

Uh oh!

markshannon commented Sep 9, 2022

Uh oh!

Uh oh!

brandtbucher commented Oct 21, 2022

Uh oh!

markshannon commented Oct 21, 2022

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Nov 8, 2022

Uh oh!

brandtbucher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GH-96421: Insert shim frame on entry to interpreter#96319
markshannon merged 39 commits into
python:mainfrom
faster-cpython:entry-frame

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

brandtbucher left a comment •
edited
Loading