-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
GH-127705: Use _PyStackRef
s in the default build.
#127875
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
Conversation
…xes to immortal objects.
…e-127705.Qad2hx.rst Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
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.
This mostly seems okay. FYI, I didn't do a deep dive into the code to verifying correctness, like I normally do. I did read through all the files except for Include/internal/pycore_stackref.h. The changes make sense and I can see how they simplify things.
@markshannon - Have you measured the effect of the mortality optimizations independently from the changes to use macros? The tag bits are a precious commodity and the mortality optimizations require two bits. |
@mpage. I changed the PR to use a single bit, using
|
@@ -1093,6 +1104,7 @@ dummy_func( | |||
inst(RETURN_VALUE, (retval -- res)) { | |||
assert(frame->owner != FRAME_OWNED_BY_INTERPRETER); | |||
_PyStackRef temp = retval; | |||
assert(PyStackRef_IsHeapSafe(temp)); |
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.
Why the assert here? This blocks stackrefs from passing through to the host frame, which blocks a lot of unboxed and deferred refcounting optimizations.
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.
It blocks a lot of unsafe operations. Passing references towards the caller is unsafe as it is possible that the only immediate reference count (stored in ob_refcnt
) for an object is stored in the callee, and will be deleted during the return.
|
|
|
|
|
|
|
|
|
We already have
_PyStackRef
s in the default build, but they are justPyObject *
pointers cast to a pointer-sized int.This PR adds tags according to the tagging scheme in #127705.