bpo-44590: Lazily allocate frame objects #27077

markshannon · 2021-07-09T11:29:12Z

This PR:

Moves all essential frame data to the (per-thread) stack
Lazily allocates heap frame objects only when needed.

For most calls to Python functions, we can get the necessary memory for a stack frame by just bumping a pointer.
When we do need a frame object, for tracebacks and the like, it is lazily allocated.

According to my benchmarking run this PR produces a maximum speedup of 31% with a mean of 7%!

The performance numbers are suspiciously good.
If someone could run the comparison on their machine that would be much appreciated.

https://bugs.python.org/issue44590

…a frame, except for the locals.

…to tstate.pyframe as preparation for lazily created frames.

…k frame.

… object outlives stack invocation.

…has been deleted.

…bject.

…t introspects frames.

Python/ceval.c

pablogsal · 2021-07-20T15:10:08Z

Python/pystate.c

@@ -2009,42 +2016,57 @@ _Py_GetConfig(void)

 #define MINIMUM_OVERHEAD 1000

-PyObject **
-_PyThreadState_PushLocals(PyThreadState *tstate, int size)
+_Py_NO_INLINE PyObject **


Why _Py_NO_INLINE?

This is the slow path. We want to make sure the fast path is inlined.

pablogsal · 2021-07-20T15:14:46Z

Python/pystate.c

-PyObject **
-_PyThreadState_PushLocals(PyThreadState *tstate, int size)
+_Py_NO_INLINE PyObject **
+_PyThreadState_PushChunk(PyThreadState *tstate, int size)


If we are going to take pointers to arbitrary places in memory, doesn't the standard require that those must be aligned? How are we making sure that all pointers to this region are aligned?. From the standard:

A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type. If the resulting pointer is not correctly aligned for the referenced type, the behavior is undefined.

Many compilers normally assume aligned pointer access.

Why do you think the returned memory would be unaligned?
How is this any different from malloc?

Why do you think the returned memory would be unaligned?

My question is not if the returned memory would be unaligned, but if we are taking pointers to random places of the chunk array. For instance, if we are doing anything isomorphic to this:

data = (char *)malloc(16); sp1 = (short*)(data+3);

then we are doing it wrong. I am asking not because I think we are doing it wrong, but because i am still wrapping around the structure you have in mind for the chunks, because there is 3 or 4 layers of indirections, a "chunk struct" and at least 3 pointer accesses when pushin frames, so immediately I am wondering if this mechanism is taking this correctly.

I compiled Python with the undefined behavior sanitizer and it doesn't complain, so seems that there is no problem here.

On contrast, my example shows:

example.c:1313:30: runtime error: load of misaligned address 0x55f1ba0cd283 for type 'short int', which requires 2 byte alignment 0x55f1ba0cd283: note: pointer points here 00 3d 0e 0b e5 f4 55 00 00 00 00 00 00 00 00 00 00 09 09 49 42 4d 39 30 36 21 00 00 00 00 00 00

On the other hand it found this (unrelated to this PR):

home/pablogsal/github/python/main/Modules/_sha3/kcp/KeccakP-1600-opt64.c:467:9: runtime error: load of misaligned address 0x563579d3c3f7 for type 'UINT64', whic h requires 8 byte alignment 0x563579d3c3f7: note: pointer points here 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 ^ /home/pablogsal/github/python/main/Modules/_sha3/kcp/KeccakP-1600-opt64.c:467:9: runtime error: load of misaligned address 0x563579d3c3ff for type 'UINT64', whic h requires 8 byte alignment 0x563579d3c3ff: note: pointer points here 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 ^ /home/pablogsal/github/python/main/Modules/_sha3/kcp/KeccakP-1600-opt64.c:467:9: runtime error: load of misaligned address 0x563579d3c407 for type 'UINT64', whic h requires 8 byte alignment 0x563579d3c407: note: pointer points here 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 ^ /home/pablogsal/github/python/main/Modules/_sha3/kcp/KeccakP-1600-opt64.c:467:9: runtime error: load of misaligned address 0x563579d3c40f for type 'UINT64', whic h requires 8 byte alignment 0x563579d3c40f: note: pointer points here 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 ^

I will open an issue

pablogsal · 2021-07-20T15:15:47Z

Python/pystate.c

+    if (new == NULL) {
+        return NULL;
+    }
+    tstate->datastack_chunk->top = tstate->datastack_top - &tstate->datastack_chunk->data[0];


Suggested change

tstate->datastack_chunk->top = tstate->datastack_top - &tstate->datastack_chunk->data[0];

tstate->datastack_chunk->top = tstate->datastack_top - tstate->datastack_chunk->data;

I am missing anything here?

No. It's just a hint to the reader that data is an array within the chunk.
chunk->data looks like a pointer deference, but &chunk->data[0] hints that this is an address calculation.

Python/pystate.c

pablogsal

LGTM Great job!

Let's run the buildbots and land!

bedevere-bot · 2021-07-26T06:43:21Z

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

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

markshannon · 2021-07-26T10:17:42Z

The failures on buildbot/AMD64 Windows10 PR look like something is wrong with that machine.

ncoghlan · 2021-08-01T01:22:38Z

One consequence of this change is that the code is now quite confusing, since the term "frame" is used ambiguously to refer to both frame objects and interpreter frames (specifically, the f_frame field on frame objects is now an interpreter frame reference, while tb_frame on tracebacks is still a full frame object).

ncoghlan · 2021-08-01T01:29:30Z

The generator code uses gi_xframe rather than gi_frame to make the distinction a bit clearer, and I think doing that systematically (so there is consistent code level distinction between execution frames and introspection frames) would make the new code much easier to follow.

ncoghlan · 2021-08-01T01:50:15Z

I've posted a proposed internal API refactoring design here: https://bugs.python.org/issue44800

ncoghlan · 2021-08-01T08:41:29Z

And the PR implementing that proposed refactoring is up at #27525

merwok · 2021-10-12T02:44:10Z

Modules/_tracemalloc.c

@@ -3,7 +3,7 @@
 #include "pycore_pymem.h"         // _Py_tracemalloc_config
 #include "pycore_traceback.h"
 #include "pycore_hashtable.h"
-#include "frameobject.h"          // PyFrame_GetBack()
+#include <pycore_frame.h>


I’m curious why this one is included with <> when all the headers above use ""
(I don’t know much C note!)

markshannon added 30 commits June 21, 2021 18:50

Turn specials array into struct and add 'lasti' to it.

79aeaf6

Move stack-depth from frame object to specials struct.

3b6a4e8

Rename 'specials' to 'frame' as it now includes most of the data for …

6f0ba40

…a frame, except for the locals.

Refactor, pushing PyFrame upward toward _PyEval_Vector.

8543c27

Add pointer from stack frame to frame object and rename tstate.frame …

423d403

…to tstate.pyframe as preparation for lazily created frames.

More refactoring. Add tstate.frame for the top stack frame.

7bf6c30

Convert use of PyFrameObject to _PyFrame.

861a8d9

Replace most remaining uses frame object in the interpreter with stac…

35e793c

…k frame.

Convert more uses of frameobject to frame.

192094e

Move f_state from frame object to frame.

3f601a7

Compute f_back when on thread stack, only filling in value when frame…

bd95c32

… object outlives stack invocation.

Add NULL check

9961abc

Get lazy f_back working (it still leaks).

32af707

Use frames not frameobjects in sys._getframe()

ac7dbe8

NULL out frame->previous when leaving frame.

f33d291

Frames now include nlocalspuls, so they have valid layout after code …

5c23a36

…has been deleted.

Move ownership of frame in generator from frame object ot generator o…

910e991

…bject.

Remove localsptr field from frame object.

22e1c9b

Add new _PyEval_EvalNoFrame function for evaluating frames directly.

f84a3f0

Allow for lazily created frames.

1180a44

Do not create frame objects for Python calls.

c76de89

Don't create frame objects for generators.

15aeef1

Fix memory leak

1d2e1ce

Merge branch 'main' into lazy-frame-updated

1b19f8b

Restore support for PEP 523.

d619bae

Streamline pushing and popping stack frames a bit.

d147d03

Merge branch 'main' into lazy-frame

25c6a71

Add f_ prefix back to several frame fields to ease porting C code tha…

618b094

…t introspects frames.

Add NEWS

e5da338

Remove debugging artifact.

dda0b0c

pablogsal reviewed Jul 20, 2021

View reviewed changes

Python/ceval.c Outdated Show resolved Hide resolved

pablogsal reviewed Jul 20, 2021

View reviewed changes

Python/pystate.c Outdated Show resolved Hide resolved

markshannon added 2 commits July 21, 2021 12:15

Various small clarifications as suggested by Pablo.

90ed5b6

Refactor interpreter frame code into its own file. Improve a few names.

593a348

markshannon requested a review from a team as a code owner July 21, 2021 12:26

markshannon added 2 commits July 21, 2021 14:20

Tidy up assert.

b775f13

Fix warning on Windows.

e8476b2

pablogsal approved these changes Jul 22, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 22, 2021

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2021

markshannon merged commit ae0a2b7 into python:main Jul 26, 2021

bedevere-bot removed the awaiting merge label Jul 26, 2021

markshannon deleted the lazy-frame branch September 15, 2021 11:51

merwok reviewed Oct 12, 2021

View reviewed changes

brandtbucher mentioned this pull request Dec 15, 2021

C extensions can't swap out live frames in CPython 3.11 python-greenlet/greenlet#281

Closed

markshannon mentioned this pull request Apr 21, 2022

Create frame objects lazily when needed #88756

Closed

The-Compiler mentioned this pull request May 26, 2022

Python/pystate.c:2218: _PyThreadState_PopFrame: Assertion `tstate->datastack_top >= base' failed. #93252

Closed

kumaraditya303 mentioned this pull request Jun 6, 2022

Add coverage benchmark python/pyperformance#213

Merged

andersk mentioned this pull request Jun 28, 2023

Segmentation fault in 3.11.4, 3.12.0b3; _PyInterpreterFrame ownership issue #106092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-44590: Lazily allocate frame objects #27077

bpo-44590: Lazily allocate frame objects #27077

markshannon commented Jul 9, 2021 •

edited

Loading

pablogsal Jul 20, 2021

markshannon Jul 21, 2021

pablogsal Jul 20, 2021 •

edited

Loading

markshannon Jul 21, 2021 •

edited

Loading

pablogsal Jul 21, 2021 •

edited

Loading

pablogsal Jul 21, 2021

pablogsal Jul 21, 2021

pablogsal Jul 20, 2021

pablogsal Jul 20, 2021

markshannon Jul 21, 2021

pablogsal left a comment

bedevere-bot commented Jul 26, 2021

markshannon commented Jul 26, 2021

ncoghlan commented Aug 1, 2021 •

edited

Loading

ncoghlan commented Aug 1, 2021

ncoghlan commented Aug 1, 2021

ncoghlan commented Aug 1, 2021

merwok Oct 12, 2021

	tstate->datastack_chunk->top = tstate->datastack_top - &tstate->datastack_chunk->data[0];
	tstate->datastack_chunk->top = tstate->datastack_top - tstate->datastack_chunk->data;

bpo-44590: Lazily allocate frame objects #27077

bpo-44590: Lazily allocate frame objects #27077

Conversation

markshannon commented Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

markshannon Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

pablogsal Jul 21, 2021 • edited Loading

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

pablogsal left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 26, 2021

markshannon commented Jul 26, 2021

ncoghlan commented Aug 1, 2021 • edited Loading

ncoghlan commented Aug 1, 2021

ncoghlan commented Aug 1, 2021

ncoghlan commented Aug 1, 2021

Choose a reason for hiding this comment

markshannon commented Jul 9, 2021 •

edited

Loading

pablogsal Jul 20, 2021 •

edited

Loading

markshannon Jul 21, 2021 •

edited

Loading

pablogsal Jul 21, 2021 •

edited

Loading

ncoghlan commented Aug 1, 2021 •

edited

Loading