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

markshannon · 2022-08-26T17:45:50Z

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.

sweeneyde · 2022-09-08T17:39:42Z

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 · 2022-09-09T16:06:59Z

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

…ut frame.

markshannon · 2022-10-20T15:19:07Z

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

The more negative benchmarks seem to use generators a fair bit, e.g https://github.com/python/pyperformance/blob/main/pyperformance/data-files/benchmarks/bm_nqueens/run_benchmark.py#L27 This PR is an enabler of optimizing entering and exiting generators and coroutines, so we would expect a win in the longer term on those benchmarks.
I expect further benefits from this change once GH-91095: Specialize calls to normal python classes #93221 is merged.
In the long term we expect calls to _PyEval_EvalFrameDefault() to become rare, as we move more control into bytecode.

brandtbucher

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.

Misc/NEWS.d/next/Core and Builtins/2022-09-09-10-37-34.gh-issue-96421.cyg33z.rst

Lib/opcode.py

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

Objects/frameobject.c

brandtbucher · 2022-10-20T23:25:09Z

Python/pylifecycle.c

+static const char INTERPRETER_TRAMPOLINE_CODE[] = {
+    0, 0,
+    INTERPRETER_EXIT, 0,
+    RESUME, 0


Why is this here?

So that this frame is incomplete. I'll add comments.

I don't think it's needed, though. co->_co_firsttraceable == Py_SIZE(co) if no RESUME is present, right?

Technically yes, but that's really an implementation detail. The rule is that a frame is invalid until RESUME or RETURN_GENERATOR has been reached.

Python/traceback.c

Objects/codeobject.c

Python/pylifecycle.c

bedevere-bot · 2022-10-20T23:25:14Z

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

markshannon · 2022-10-21T16:57:26Z

I have made the requested changes; please review again

bedevere-bot · 2022-10-21T16:57:31Z

Thanks for making the requested changes!

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

brandtbucher · 2022-10-21T17:01:20Z

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 · 2022-10-21T17:19:26Z

It can wait until next week

brandtbucher

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

brandtbucher · 2022-10-31T04:46:11Z

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

+related C-API function, a shim frame in inserted into the call stack.
+This occurs in the ``_PyEval_EvalFrameDefault()`` function.
+The extra frame should be invisible to all Python and most C extensions,
+but out-of-process debuggers, profilers and debuggers need to be aware of


Suggested change

but out-of-process debuggers, profilers and debuggers need to be aware of

but out-of-process profilers and debuggers need to be aware of

brandtbucher · 2022-10-31T04:46:11Z

Objects/codeobject.c

+    if (co_code == NULL) {
+        goto cleanup;
+    }
+    int code_units = codedef->codelen/2;


Suggested change

int code_units = codedef->codelen/2;

int code_units = codedef->codelen / sizeof(_Py_CODEUNIT)

brandtbucher · 2022-10-31T04:46:11Z

Objects/codeobject.c

+        goto cleanup;
+    }
+    int code_units = codedef->codelen/2;
+    int loc_entries = (code_units + 7)/8;


Maybe add a comment like "The maximum number of code units encoded by a single line table entry is 8. This is equivalent to ceil(code_units / 8)."

brandtbucher · 2022-10-31T04:46:11Z

Objects/codeobject.c

+    loc_table = PyMem_Malloc(loc_entries);
+    if (loc_table == NULL) {
+        PyErr_NoMemory();
+        return NULL;


Suggested change

return NULL;

goto cleanup;

brandtbucher · 2022-10-31T04:46:11Z

Objects/codeobject.c

+    if (loc_table) {
+        PyMem_Free(loc_table);
+    }


This can be done above without the conditional.

Suggested change

if (loc_table) {

PyMem_Free(loc_table);

}

brandtbucher · 2022-10-31T04:46:11Z

Objects/codeobject.c

+    assert(code_units > 0 && code_units <= 8);
+    loc_table[loc_entries-1] = 0x80 |
+        (PY_CODE_LOCATION_INFO_NONE << 3) | (code_units-1);
+    lines = PyBytes_FromStringAndSize((const char *)loc_table, loc_entries);


Suggested change

lines = PyBytes_FromStringAndSize((const char *)loc_table, loc_entries);

lines = PyBytes_FromStringAndSize((const char *)loc_table, loc_entries);

PyMem_Free(loc_table);

brandtbucher · 2022-10-31T04:46:11Z

Objects/codeobject.c

+        goto cleanup;
+    }
+    struct _PyCodeConstructor con = {
+        .filename = &_Py_STR(empty),


Not a huge deal, but I think we typically put something here for ease of debugging. Maybe "<internal>"?

gvanrossum

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.

gvanrossum · 2022-11-03T22:32:10Z

Lib/test/test_subprocess.py

+    @skip_if_sanitizer(memory=True, address=True,
+                       reason= "Spurious error when assigning to stack variable.")


gvanrossum · 2022-11-03T22:32:10Z

Objects/frameobject.c

@@ -1326,14 +1326,18 @@ PyFrame_LocalsToFast(PyFrameObject *f, int clear)
    }
 }

-
 int _PyFrame_IsEntryFrame(PyFrameObject *frame)


Suggested change

int _PyFrame_IsEntryFrame(PyFrameObject *frame)

int

_PyFrame_IsEntryFrame(PyFrameObject *frame)

gvanrossum · 2022-11-03T22:32:10Z

Objects/frameobject.c

+    if (f->previous == NULL) {
+        return 0;
+    }
+    return f->previous->owner == FRAME_OWNED_BY_CSTACK;


Suggested change

if (f->previous == NULL) {

return 0;

}

return f->previous->owner == FRAME_OWNED_BY_CSTACK;

return f->previous != NULL && f->previous->owner == FRAME_OWNED_BY_CSTACK;

gvanrossum · 2022-11-03T22:32:10Z

Objects/genobject.c

    /* Don't keep the reference to previous any longer than necessary.  It
     * may keep a chain of frames alive or it could create a reference
     * cycle. */


Update comment.

gvanrossum · 2022-11-03T22:32:10Z

Python/pylifecycle.c

+static const char INTERPRETER_TRAMPOLINE_CODE[] = {
+    0, 0,
+    INTERPRETER_EXIT, 0,
+    RESUME, 0
+};


Why is the code separate from the MakeTrampoline function, whereas other details are hardcoded there? Why not hardcode this there as well?

Python/pylifecycle.c

gvanrossum · 2022-11-03T22:32:10Z

Python/ceval.c

@@ -1852,27 +1862,33 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
            goto error;
        }

-        TARGET(RETURN_VALUE) {


Reminder that you'll get a merge conflict here -- instruction changes now need to be in bytecodes.c and you need to run make regen-cases.

markshannon added 3 commits Aug 24, 2022

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 Aug 26, 2022

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 Aug 30, 2022

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 Aug 31, 2022

Use typedef name.

06c2ad6

markshannon force-pushed the entry-frame branch from 551a66e to 06c2ad6 Compare Aug 31, 2022

markshannon added 4 commits Sep 5, 2022

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

Open

markshannon added 3 commits Sep 9, 2022

Remove extra semicolons.

0db3b25

Add news

05b4f68

Merge branch 'main' into entry-frame

6cca61c

sweeneyde reviewed Sep 11, 2022

View changes

Python/pylifecycle.c Show resolved Hide resolved

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

Open

4 tasks

markshannon added 3 commits Sep 14, 2022

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

markshannon added 2 commits Oct 19, 2022

Add news and update frame_layout.md

f7072e1

Merge branch 'main' into entry-frame

52406d2

markshannon marked this pull request as ready for review Oct 19, 2022

markshannon requested review from gpshead and iritkatriel as code owners Oct 19, 2022

markshannon added 2 commits Oct 20, 2022

Remove debugging scaffolding and give C compiler more freedom to layo…

bd2b0ee

…ut frame.

Remove typo

10b03c8

brandtbucher requested changes Oct 20, 2022

View changes

bedevere-bot added awaiting changes and removed awaiting core review labels Oct 20, 2022

markshannon added 7 commits Oct 21, 2022

Address code review comments.

b62f5f3

Generate linetable when creating shim code.

1a0b08f

Bundle shim code definition into single struct.

76e437a

Remove incorrect assert.

945e26a

assert generator is cleared after returning or raising.

676321a

Rename pyframe to entry_frame.

3cb22e6

Pass correct length.

e5dcbd9

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 21, 2022

bedevere-bot requested a review from brandtbucher Oct 21, 2022

brandtbucher mentioned this pull request Oct 28, 2022

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

Draft

brandtbucher reviewed Oct 31, 2022

View changes

markshannon mentioned this pull request Oct 31, 2022

GH-96793: Specialize FOR_ITER for generators. #98772

Open

gvanrossum reviewed Nov 3, 2022

View changes

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

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

markshannon commented Aug 26, 2022 •

edited by bedevere-bot

sweeneyde commented Sep 8, 2022

markshannon commented Sep 9, 2022

markshannon commented Oct 20, 2022

brandtbucher left a comment

brandtbucher Oct 20, 2022

markshannon Oct 21, 2022

brandtbucher Oct 31, 2022

markshannon Nov 3, 2022

bedevere-bot commented Oct 20, 2022

markshannon commented Oct 21, 2022

bedevere-bot commented Oct 21, 2022

brandtbucher commented Oct 21, 2022

markshannon commented Oct 21, 2022

brandtbucher left a comment

brandtbucher Oct 31, 2022

brandtbucher Oct 31, 2022

brandtbucher Oct 31, 2022

brandtbucher Oct 31, 2022

brandtbucher Oct 31, 2022

brandtbucher Oct 31, 2022

brandtbucher Oct 31, 2022

gvanrossum left a comment

gvanrossum Nov 3, 2022

gvanrossum Nov 3, 2022

gvanrossum Nov 3, 2022

gvanrossum Nov 3, 2022

gvanrossum Nov 3, 2022

gvanrossum Nov 3, 2022

	but out-of-process debuggers, profilers and debuggers need to be aware of
	but out-of-process profilers and debuggers need to be aware of

	int code_units = codedef->codelen/2;
	int code_units = codedef->codelen / sizeof(_Py_CODEUNIT)

	lines = PyBytes_FromStringAndSize((const char *)loc_table, loc_entries);
	lines = PyBytes_FromStringAndSize((const char *)loc_table, loc_entries);
	PyMem_Free(loc_table);

		@skip_if_sanitizer(memory=True, address=True,
		reason= "Spurious error when assigning to stack variable.")

	int _PyFrame_IsEntryFrame(PyFrameObject *frame)
	int
	_PyFrame_IsEntryFrame(PyFrameObject *frame)

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

Are you sure you want to change the base?

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

Conversation

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

sweeneyde commented Sep 8, 2022

markshannon commented Sep 9, 2022

markshannon commented Oct 20, 2022

brandtbucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 20, 2022

markshannon commented Oct 21, 2022

bedevere-bot commented Oct 21, 2022

brandtbucher commented Oct 21, 2022

markshannon commented Oct 21, 2022

brandtbucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon commented Aug 26, 2022 •

edited by bedevere-bot