| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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 const char* type rather than PyObject* for the variable name) which should be more convenient to use in C, but a little bit slower (create a temporary Unicode object, decode the short bytes string from UTF-8). 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? |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, I'd like to have a discussion about the API design first.
Sorry, something went wrong.
Would you prefer int PyFrame_GetVar(PyFrameObject *frame, PyObject *name, PyObject **value) API which would write into *value instead? The problem with such API is to decide what to do with the *value old value before overriding it. If we consider that it's undefined and it must not be read, the API can be misused and it can introduce a reference leak. Like: 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 *value value. 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 PyErr_ExceptionMatches(). In short, I propose: PyObject *PyFrame_GetVar(PyFrameObject *frame, PyObject *name)
In short, NULL indicates an error, and non-NULL is a success. |
Sorry, something went wrong.
That sounds like the API I was asking for, so yes! |
Sorry, something went wrong.
|
A few questions:
|
Sorry, something went wrong.
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 (...)
SELF_STRING = PyUnicode_InternFromString("self");
CLS_STRING = PyUnicode_InternFromString("cls");
(...)
#if PY_VERSION_HEX >= 0x030b0000 // Python 3.11.0
static const char *
_get_class_name_of_frame(PyFrameObject *frame, PyCodeObject *code) {
PyObject *locals = PyFrame_GetLocals(frame);
if (!PyDict_Check(locals)) { return NULL; }
PyObject *self = PyDict_GetItem(locals, SELF_STRING);
if (self) {
Py_DECREF(locals);
return _PyType_Name(self->ob_type);
}
PyObject *cls = PyDict_GetItem(locals, CLS_STRING);
if (cls) {
Py_DECREF(locals);
if (!PyType_Check(cls)) {
return NULL;
}
PyTypeObject *type = (PyTypeObject *)cls;
return _PyType_Name(type);
}
Py_DECREF(locals);
return NULL;
}
#else
(...)
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? |
Sorry, something went wrong.
|
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); |
Sorry, something went wrong.
|
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 PyFrameObject. When I iterate down the stack to capture it, I use PyFrame_GetBack, which will create the frame objects anyway. I suppose it would be better not to, but I need to get at the PyCodeObject objects anyway, and PyFrame_GetCode is the only to get them with the public API. So, for me, PyFrame_GetVar would be very useful. PyFrameStack_GetVar would only be useful if I could also get code objects with e.g. PyFrameStack_GetCode. Oh, and PyFrameStack_GetLineNumber... there might be an argument for some kind of coroutine/generator state inspection too, (@sumerc might have thoughts on this). 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 PyFrameObject objects don't appear, I can see the logic of PyFrameStack_GetStuff. Still, I think I'd need more accessors than just PyFrameStack_GetVar to get all the info that I'd want to build a profiler without frame objects. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Reminder:
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.