gh-94936: C getters: co_varnames, co_cellvars, co_freevars #95008
Conversation
Fidget-Spinner
commented
Jul 19, 2022
•
Fidget-Spinner
commented
- Issue: Public C API for accessing code object fields #94936
| return NULL; | ||
| } | ||
| /* co_varnames */ | ||
| { |
There was a problem hiding this comment.
Would it make more sense to expose the functions here, and test them in test/test_capi.py, checking that the C version returns the same as the Python version?
There was a problem hiding this comment.
It might be better, but I'm fine with these tests.
| PyObject * | ||
| PyCode_GetVarnames(PyCodeObject *code) | ||
| { | ||
| return _PyCode_GetVarnames(code); |
There was a problem hiding this comment.
What's the advantage of separate wrapper functions rather than just removing the leading _ from the wrapped functions?
There was a problem hiding this comment.
I concur wuth @gvanrossum, unless it's implemented as a static inline function, I see no advantage, but that's not the case here. Just rename _PyCode_GetVarnames() to PyCode_GetVarnames(). Same remark for the 2 other functions.
| PyObject * | ||
| PyCode_GetVarnames(PyCodeObject *code) | ||
| { | ||
| return _PyCode_GetVarnames(code); |
There was a problem hiding this comment.
I concur wuth @gvanrossum, unless it's implemented as a static inline function, I see no advantage, but that's not the case here. Just rename _PyCode_GetVarnames() to PyCode_GetVarnames(). Same remark for the 2 other functions.
| return NULL; | ||
| } | ||
| /* co_varnames */ | ||
| { |
There was a problem hiding this comment.
It might be better, but I'm fine with these tests.
|
This change targets Python 3.11 which no longer accepts new features (after beta1). But it seems like the lack of public C API functions to the removed PyCodeObject members is an issue, see: https://discuss.python.org/t/getting-the-class-name-of-a-code-frame-object-in-cpython-3-11-c-api/17396 Should we target Python 3.11 or is it ok to leave Python 3.11 with no public C API for that? |
|
@pablogsal -- These three new C getter APIs would be useful for some and don't really add any risk for 3.11, but at the same time I don't think they are essential. What do you think? |
A datapoint: when I proposed to replace a direct access to a structure member in C with PyObject_GetAttrString(), @nedbat reported a serious slowdown in his https://github.com/nedbat/coveragepy/ project. PyFrame_GetLasti() was added to Python 3.11 to solve this performance issue: https://docs.python.org/dev/c-api/frame.html#c.PyFrame_GetLasti I failed to find the discussion about the performance regression caused by PyObject_GetAttrString() but it's obviously way more work than just reading directly a C structure member... The function decodes a bytes string from UTF-8, creates a temporary Unicode object, lookup in a dictionary, delete the temporary Unicode code... But then you get a Python object, you need to unbox its content and delete the second (temporary?) result object... |
|
FWIW one suggestion (see #94936 (comment)) to avoid the perf regression of |
I would be ok landing this in 3.11 but we are one day from an already delayed release candidate so if these APIs are not essential I would prefer delaying them to 3.12 because they won't be tested when we release them (before the RC) and I'm not sure of we can properly review the implementation in one day. If you think is going to be much better to land these in 3.11 then let's do it, but we are going to need at least 2 core Devs to review and approve the PR. |
There was a problem hiding this comment.
LGTM. @gvanrossum: Would you mind to review this PR as well?
I can rename _PyCode_GetVarnames() to PyCode_GetVarnames() in a following PR. But I would prefer to see these functions added to Python 3.11. Avoiding the internal API to access code members in C is way more convenient to port extensions to Python 3.11!
|
Thanks @Fidget-Spinner for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
…honGH-95008) (cherry picked from commit 42b102b) Co-authored-by: Ken Jin <[email protected]>
|
GH-95653 is a backport of this pull request to the 3.11 branch. |
|
I wrote python/pythoncapi-compat#44 to add the 3 functions to pythoncapi-compat (for Python 3.10 and older). |
(cherry picked from commit 42b102b) Co-authored-by: Ken Jin <[email protected]>
|
Oops I missed the convo on this one.I planned to direct extension authors to use |
|
Thank you so much for getting this in! 🙏 |
