New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-117139: Convert the evaluation stack to stack refs #118450
base: main
Are you sure you want to change the base?
Conversation
Could we hold off on this until 3.14? It's only a week until feature freeze for 3.13 (at which point main becomes 3.14), and this looks like a lot of churn in a time where we all would like stability to merge things that are actually needed in 3.13. |
Alright. I always forget that ten other people are rushing in things at the same time as me. I don't think this will add any value for CPython 3.13 anyways at the moment. |
That makes sense. I'll start providing feedback and reviewing this now, but it won't be merged in 3.13. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments below, mostly minor. I'll take a closer look at bytecodes.c
and the generated cases next.
I have made the requested changes; please review again Namely:
To be conservative, this PR only defers immortal objects at the moment and NULL. |
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
static inline PyObject * | ||
PyStackRef_Get(_PyStackRef tagged) | ||
PyStackRef_To_PyObject_Borrow(_PyStackRef tagged) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These names do not look like other CPython function naming conventions to me. Can we do something like:
PyStackRef_AsPyObjectBorrow(_PyStackRef)
-- instead of this funcion
And, for example:
PyStackRef_AsPyObjectSteal
PyStackRef_FromPyObjectBorrow
PyStackRef_FromPyObjectSteal
Note: I'm aware that certain decref options don't follow the semantics laid out in the issue. For now they're still safe because they will never operate on deferred objects. In PEP 703's plan, (non-immortal) floats and ints will not be deferred. So we're safe for now. However, in the future if we want to defer everything, then those need to be removed #118930. |
5% performance hit on single-threaded default build: https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20240511-3.14.0a0-144a6fa |
I really think we need a design document (in an issue) that we can discuss before doing more work on this.
|
I'll add a more detailed summary of the GC changes to:
We don't look at non-deferred pointers above To be clear, what we are describing here is an extra step in the GC that scans each thread's stack and ensures that deferred referenced objects from thread's stacks are kept alive. These frames are mostly not tracked and would not otherwise be considered by the GC. We don't need to do anything special in this step for non-deferred references because those objects are kept alive by their reference count (i.e., computed |
I've updated #117376. |
Only tags all pointers 0b11 and NULL and immortal stuff as deferred for now.