bpo-44032: Move data stack to thread from FrameObject.#26076
bpo-44032: Move data stack to thread from FrameObject.#26076markshannon merged 21 commits intopython:mainfrom
Conversation
…ocals + frees + cells.
… and slow implementation.
…y use most of the time.
markshannon
force-pushed
the
datastack-on-thread
branch
from
470b206 to
d9936c3
Compare
markshannon
force-pushed
the
datastack-on-thread
branch
from
d9936c3 to
2c14939
Compare
ericsnowcurrently
left a comment
ericsnowcurrently
left a comment
There was a problem hiding this comment.
Overall, looks good and the value of this change seems pretty clear. 🙂
I'll probably take a second pass before approving. I mostly follow the change, but mapping the old code to the new conceptually isn't easy. The old code was fairly straight-forward to follow. The new code isn't quite so easy. It may help to have a bit more explanation somewhere of how the threadstate-based stack operates, including relative to frames.
| PyObject *co_name; /* unicode (name, for reference) */ | ||
| PyObject *co_linetable; /* string (encoding addr<->lineno mapping) See | ||
| Objects/lnotab_notes.txt for details. */ | ||
| int co_nlocalsplus; /* Number of locals + free + cell variables */ |
There was a problem hiding this comment.
FWIW, I've been calling this co_nfastlocals in my branches.
There was a problem hiding this comment.
But it isn't all "fast" locals, as there are cells as well. Admittedly, co_nlocalsplus isn't a great name either.
There was a problem hiding this comment.
"fastlocals" is what we already call them in ceval.c.
|
When you're done making the requested changes, leave the comment: |
ericsnowcurrently
left a comment
ericsnowcurrently
left a comment
There was a problem hiding this comment.
LGTM
After some offline discussion it makes sense now. We would still benefit from some explanation in the code though.
markshannon
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @markshannon for commit c5ccb7d 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
The failure was a timeout on test_multiprocessing_fork one of the refleak buildbots. |
| { | ||
| assert(f->f_own_locals_memory == 0); | ||
| assert(f->f_stackdepth == 0); | ||
| int size = frame_nslots(f); |
There was a problem hiding this comment.
@markshannon, this gives a compiler error because frame_nslots() returns a Py_ssize_t.
5 participants
Move local, cell and free variables, plus the evaluation stack to the thread.
Cuts down the size of frames to under 100 bytes, and enables further optimizations of Python-to-Python calls.
https://bugs.python.org/issue44032