◐ Shell
reader mode source ↗
Skip to content

bpo-44032: Move data stack to thread from FrameObject.#26076

Merged
markshannon merged 21 commits into
python:mainfrom
faster-cpython:datastack-on-thread
May 21, 2021
Merged

bpo-44032: Move data stack to thread from FrameObject.#26076
markshannon merged 21 commits into
python:mainfrom
faster-cpython:datastack-on-thread

Conversation

@markshannon

@markshannon markshannon commented May 12, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

markshannon added 14 commits May 12, 2021 16:09
@markshannon
Remove 'zombie' frames. We won't need them once we are allocating fix… …
65349e5 
…ed-size frames.
@markshannon
Add co_nlocalplus field to code object to avoid recomputing size of l… …
c1ac3ef 
…ocals + frees + cells.
@markshannon
Move locals, cells and freevars out of frame object into separate mem… …
431e1cb 
…ory buffer.
@markshannon
Use per-threadstate allocated memory chunks for local variables. Dumb… …
24a77d8 
… and slow implementation.
@markshannon
Make per-thread data-stack a contiguous block of memory.
19ac31a
@markshannon
Add comments about data stack sizes.
2d1a9db
@markshannon
Use chunked stack, allows larger stack when needed with reduced memor… …
73c49d5 
…y use most of the time.
@markshannon
Delete obsolete comment and debug print statements
8a9ae01
@markshannon
Move globals and builtins from frame object to per-thread stack.
5a4803c
@markshannon
Move (slow) locals frame object to per-thread stack.
a555393
@markshannon
Add back comment.
f238598
@markshannon
Fix limit when popping block from data stack.
6cab045
@markshannon
Tidy up frame creation a bit.
89f9496
@markshannon
Improve function name
0091341
@markshannon markshannon requested a review from 1st1 as a code owner May 12, 2021 16:58
@markshannon markshannon force-pushed the datastack-on-thread branch from 470b206 to d9936c3 Compare May 13, 2021 13:10
@markshannon markshannon force-pushed the datastack-on-thread branch from d9936c3 to 2c14939 Compare May 13, 2021 15:21
@ericsnowcurrently ericsnowcurrently self-requested a review May 17, 2021 16:31
ericsnowcurrently
ericsnowcurrently requested changes May 19, 2021
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bedevere-bot

bedevere-bot commented May 19, 2021

Copy link
Copy Markdown

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

ericsnowcurrently
ericsnowcurrently approved these changes May 20, 2021
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM

After some offline discussion it makes sense now. We would still benefit from some explanation in the code though.

@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 20, 2021
@bedevere-bot

bedevere-bot commented May 20, 2021

Copy link
Copy Markdown

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

@markshannon

markshannon commented May 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

The failure was a timeout on test_multiprocessing_fork one of the refleak buildbots.
The other refleak buildbots passed though, so there are no leaks (at least not that are caught by the buildbots).

@markshannon markshannon merged commit b11a951 into python:main May 21, 2021
@markshannon markshannon deleted the datastack-on-thread branch August 23, 2021 13:32
@brandtbucher brandtbucher mentioned this pull request Dec 15, 2021
Closed
@CyberDem0n CyberDem0n mentioned this pull request Jan 9, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

@1st1 1st1 Awaiting requested review from 1st1

Assignees

@ericsnowcurrently ericsnowcurrently

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@markshannon @bedevere-bot @ericsnowcurrently @gvanrossum @the-knights-who-say-ni