-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-91248: Add PyFrame_GetVar() function #95712
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
Since PyCode_GetVarnames(), PyCode_GetCellvars() and PyCode_GetFreevars() got added to Python 3.11 (PR #95008), I propose a concrete implementation of my proposed PyFrame_GetVar() API. The PR also adds a PyFrame_GetVarString() API (use For now, my implementation uses PyFrame_GetLocals() + PyDict_GetItemWithError() which is not optimum. The implementation can be optimized later. I would prefer to get an agreement on the proposed API and land this PR, and then optimize it. @Fidget-Spinner @gvanrossum: Would you mind to review my PR? |
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.
LGTM.
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.
Sorry, I'd like to have a discussion about the API design first.
Would you prefer value = Py_NewRef(Py_None); // default value
(void)PyFrame_GetVar(frame, name, &value);
// leak a reference on None singleton
I'm not a big fan of having to check for PyErr_Occurred() to know if an exception was raised when NULL was returned. But the advantage of such API is to avoid the question of what doing with the About this specific API, I'm fine with raising NameError if the frame has no variable called name. This API should not be used to check if a variable exists or not. And checking for NameError is trivial in C with In short, I propose:
In short, NULL indicates an error, and non-NULL is a success. |
That sounds like the API I was asking for, so yes! |
A few questions:
|
Well, first I want it :-) As I explained in the issue #91248, this function adds again a feature indirectly removed in Python 3.11. Well, Python 3.11 adds PyFrame_GetLocals() which gives a similar feature, but it's just less efficient. The idea is to provide a similar feature than what we had in Python 3.10, with similar performance. My plan is to avoid the creation of a temporary dictionary for the final implementation, to have best performances.
I see projects working directly on frame objects and I don't think that they can easily move away from their design which worked well on Python 3.10 and older. For example, here is a PR for pyinstrument which gets a class name from a frame object: https://github.com/joerick/pyinstrument/pull/203/files
I have no idea how to get a variable by its name. Also, in Python 3.10 and older, PyFrameObject.f_locals gave access to a dictionary where keys are variable names. For me, it's more natural to get a variable by its name, than by an index. Maybe a separated API, even more efficient, can be added for best performance? |
This API forces requires two steps instead of one, and forces the creation of the frame object. PyFrameObject *frame = PyThreadState_GetFrame(tstate) /* This is expensive */
PyObject foo = PyFrame_GetVar(frame, foo);
Py_DECREF(frame); What it I would prefer: PyObject *foo = PyFrameStack_GetVar(tstate, 0, foo); |
Hi all, pyinstrument maintainer here, thanks for this PR and discussion! I just thought I'd chime in from a profiler perspective on this API design question-
In my case, in examining the stack from a setprofile callback, I already have a So, for me, It would also be a decent refactor for me to move to a PyThreadState-based API, since all the code in the project uses frame objects currently. That isn't to say that it wouldn't be worth it however! I just don't know yet what the perf differences would be. Another angle on this - I just read @markshannon's PEP 669 - from that standpoint, where |
I have written all the requirements I have for two tracing profilers I maintain (yappi and Blackfire) here: I don't have any strong opinions on function prototypes that are discussed here. I am just hoping for:
I know from first hand how much performance difference can these functions make, especially in a tracing profiler where there are lots of calls happening. |
Add PyFrame_GetVar() and PyFrame_GetVarString() functions to get a frame variable by its name. Move PyFrameObject C API tests from test_capi to test_frame.
@gvanrossum: I updated my PR to raise a NameError if the variable doesn't exist. Would you mind to review the updated PR? |
I prepared python/pythoncapi-compat#46 which implements PyFrame_GetVar() and PyFrame_GetVarString() on Python 3.11 and older. I also wrote it to prove that it's possible to implement this function on old Python versions. |
Reminder:
|
Ok, I merged my PR. As I wrote, IMO it doesn't prevent adding PyFrameStack_GetVar(). But PyFrame_GetVar() fills a hole created in Python 3.11 C API when PyFrameObject structure was made internal. Now we can see how to optimize PyFrame_GetVar() :-)
Merged. |
Add PyFrame_GetVar() and PyFrame_GetVarString() functions to get a
frame variable by its name.
Move PyFrameObject C API tests from test_capi to test_frame.