gh-94936: C getters: co_varnames, co_cellvars, co_freevars by Fidget-Spinner · Pull Request #95008 · python/cpython · GitHub

Issue: Public C API for accessing code object fields #94936

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?

It might be better, but I'm fine with these tests.

What's the advantage of separate wrapper functions rather than just removing the leading _ from the wrapped functions?

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.

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?

but at the same time I don't think they are essential

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 PyObject_GetAttrString() is to just include internal headers and call the _Py versions of the functions. That would have to be done just for 3.11. Of course a shortcut would also be to just duplicate the extern declarations for these from the headers. Again just for 3.11.

@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?

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.

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!

Since it’s urgent I’m fine with this as is. We can clean up later.

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 <28750310+Fidget-Spinner@users.noreply.github.com>

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 <28750310+Fidget-Spinner@users.noreply.github.com>

Oops I missed the convo on this one.I planned to direct extension authors to use PyObject_GetAttrString. I want a better/more performant implementation for 3.12. It will cache the debug info in a lazy debug struct much like how we do it for PyInterpreterFrame and PyFrameObject now.

Thank you so much for getting this in! 🙏

C getters: co_varnames, co_cellvars, co_freevars

d287d7c

Fidget-Spinner requested a review from markshannon as a code owner July 19, 2022 14:27

bedevere-bot added the awaiting core review label Jul 19, 2022

Add news entry

a1450f8

Fidget-Spinner requested a review from vstinner July 19, 2022 14:38

markshannon reviewed Jul 19, 2022

View reviewed changes

gvanrossum mentioned this pull request Jul 20, 2022

Public C API for accessing code object fields #94936

Closed

gvanrossum reviewed Jul 20, 2022

View reviewed changes

vstinner reviewed Aug 3, 2022

View reviewed changes

vstinner approved these changes Aug 4, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 4, 2022

vstinner added the needs backport to 3.11 only security fixes label Aug 4, 2022

gvanrossum approved these changes Aug 4, 2022

View reviewed changes

gvanrossum merged commit 42b102b into python:main Aug 4, 2022

bedevere-bot removed the awaiting merge label Aug 4, 2022

bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 4, 2022

miss-islington added a commit that referenced this pull request Aug 4, 2022

gh-94936 : C getters: co_varnames, co_cellvars, co_freevars (GH-95008 )

f292635

(cherry picked from commit 42b102b) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

Fidget-Spinner deleted the code_getters branch August 4, 2022 15:38

vstinner mentioned this pull request Aug 5, 2022

gh-91248: Add PyFrame_GetVar() function #95712

Merged

-                      return NULL;
+                  }
+                  /* co_varnames */
+                  {

+              PyObject *
+              PyCode_GetVarnames(PyCodeObject *code)
+              {
+                  return _PyCode_GetVarnames(code);

gh-94936: C getters: co_varnames, co_cellvars, co_freevars by Fidget-Spinner · Pull Request #95008 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-94936: C getters: co_varnames, co_cellvars, co_freevars - #95008

gh-94936: C getters: co_varnames, co_cellvars, co_freevars#95008gvanrossum merged 2 commits intopython:mainpython/cpython:mainfrom Fidget-Spinner:code_gettersFidget-Spinner/cpython:code_gettersCopy head branch name to clipboard

Conversation

Fidget-Spinner commented Jul 19, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 3, 2022 • edited by gvanrossum Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner Aug 3, 2022 • edited by gvanrossum Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

vstinner commented Aug 3, 2022

Uh oh!

gvanrossum commented Aug 3, 2022

Uh oh!

vstinner commented Aug 3, 2022

Uh oh!

gvanrossum commented Aug 3, 2022

Uh oh!

pablogsal commented Aug 4, 2022

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Aug 4, 2022

Uh oh!

bedevere-bot commented Aug 4, 2022

Uh oh!

vstinner commented Aug 4, 2022

Uh oh!

Fidget-Spinner commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joerick commented Aug 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gh-94936: C getters: co_varnames, co_cellvars, co_freevars#95008
gvanrossum merged 2 commits into
python:mainfrom
Fidget-Spinner:code_getters

Fidget-Spinner commented Jul 19, 2022 •
edited by bedevere-bot
Loading

vstinner Aug 3, 2022 •
edited by gvanrossum
Loading

vstinner Aug 3, 2022 •
edited by gvanrossum
Loading

Fidget-Spinner commented Aug 4, 2022 •
edited
Loading