Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
GH-127705: Use _PyStackRefs 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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
markshannon commented Dec 12, 2024 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
…xes to immortal objects.
…e-127705.Qad2hx.rst Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
ericsnowcurrently left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mpage commented Mar 6, 2025
@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. |
markshannon commented Mar 7, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@mpage. I changed the PR to use a single bit, using
|
| inst(RETURN_VALUE, (retval--res)){ | ||
| assert(frame->owner!=FRAME_OWNED_BY_INTERPRETER); | ||
| _PyStackReftemp=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.
markshannonMar 10, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
2bef8ea into python:mainUh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Mar 10, 2025
|
bedevere-bot commented Mar 10, 2025
|
bedevere-bot commented Mar 10, 2025
|
bedevere-bot commented Mar 10, 2025
|
bedevere-bot commented Mar 10, 2025
|
bedevere-bot commented Mar 10, 2025
|
bedevere-bot commented Mar 10, 2025
|
bedevere-bot commented Mar 10, 2025
|
bedevere-bot commented Mar 11, 2025
|
We already have
_PyStackRefs 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.