Uh oh!
There was an error while loading. Please reload this page.
bpo-44032: Move data stack to thread from FrameObject. - #26076
Conversation
…ocals + frees + cells.
… and slow implementation.
…y use most of the time.
470b206 to
d9936c3Compared9936c3 to
2c14939Compare
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot
commented
May 19, 2021
When you're done making the requested changes, leave the 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.
bedevere-bot
commented
May 20, 2021
🤖 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. |
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.
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