Skip to content

build: MyFrame_lasti() uses PyObject_GetAttrString() #1331

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

Closed
wants to merge 2 commits into from
Closed

build: MyFrame_lasti() uses PyObject_GetAttrString() #1331

wants to merge 2 commits into from

Conversation

vstinner
Copy link

Reimplement the MyFrame_lasti() function using
PyObject_GetAttrString(frame, "f_lasti") to avoid using the internal
C API on Python 3.11.

Add assertions in CTracer_handle_call() and CTracer_handle_return()
to ensure that f_lasti is the in expected bounds.

Reimplement the MyFrame_lasti() function using
PyObject_GetAttrString(frame, "f_lasti") to avoid using the internal
C API on Python 3.11.

Add assertions in CTracer_handle_call() and CTracer_handle_return()
to ensure that f_lasti is the in expected bounds.
@vstinner
Copy link
Author

Add assertions in CTracer_handle_call() and CTracer_handle_return() to ensure that f_lasti is the in expected bounds.

I'm not sure that this part is correct :-) But the current code doesn't seem to handle negative lasti. With my change, lasti can be negative if something goes wrong, but it should not happen in practice: a frame always has a f_lasti attribute.

@vstinner
Copy link
Author

@nedbat: Would you mind to have a look?

Do you prefer to read directly the PyFrameObject.f_lasti on Python 3.10 and older? Or are you fine with using the same code path on all Python versions? It's up to you.

#endif
static inline Py_ssize_t MyFrame_lasti(PyFrameObject *frame)
{
PyObject *obj = PyObject_GetAttrString((PyObject*)frame, "f_lasti");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I shouldn't be concerned about the overhead of this function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to measure that. If you measure that the overhead is too large, I can consider adding a getter function to Python 3.11.

Does coverage performance overhead matter in general? Usually, I don't care much about performance of debugging or profiler tools.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People run their test suites under coverage, and test suites are always too slow. "debugging and profiling" is a more occasional activity. I'll take a look at it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I get it. But I would prefer to not add getter functions just for coverage if the performance overhead of this PR is "acceptable". I would prefer to only consider new functions to the Python C API if it's "needed". So it would be nice if you could provide a benchmark (and benchmark results) so measure the performance impact of this PR. Sorry, I don't know how to do that.

@jouve
Copy link

jouve commented Apr 7, 2022

f_lasti is now gone : python/cpython#32208

@vstinner
Copy link
Author

vstinner commented Apr 7, 2022

f_lasti is now gone : python/cpython#32208

The Python frame.f_lasti field (this PR) remains accessible and valid.

@jouve
Copy link

jouve commented Apr 7, 2022

yes, I just meant this PR was all the more necessary :)

@jouve
Copy link

jouve commented Apr 7, 2022

there are also new errors related to direct access to PyFrameObject->f_trace and PyFrameObject->f_lineno :

coverage/ctracer/tracer.c:520:21: error: invalid use of incomplete typedef ‘PyFrameObject’ {aka ‘struct _frame’}

and PyCodeObject->co_code

coverage/ctracer/tracer.c:534:46: error: ‘PyCodeObject’ has no member named ‘co_code’

@nedbat
Copy link
Owner

nedbat commented Apr 7, 2022

You haven't said, but I assume you are using 3.11.0a7? I could use some help adjusting to all of the frame changes.

@jouve
Copy link

jouve commented Apr 7, 2022

PyFrameObject->f_trace and PyFrameObject->f_lineno error is from python/cpython@18b5dd6,
were struct _frame was moved from cpython/frameobject.h to internal/pycore_frame.h
tagged in v3.11.0a6

PyCodeObject->co_code is from python/cpython@2bde682
tagged in v3.11.0a7

and f_lasti removal is from master: python/cpython@ef6a482

@markshannon
Copy link

python/cpython#32413 adds PyFrame_GetLasti, which was overlooked when adding the other C-API functions.
Once the beta is released, following should work, I think. I haven't tested this at all.

#if PY_VERSION_HEX >= {hex version for 3.11 b}
#define MyFrame_lasti(f) PyFrame_GetLasti(f)

@vstinner
Copy link
Author

vstinner commented Apr 8, 2022

python/cpython#32413 adds PyFrame_GetLasti, which was overlooked when adding the other C-API functions. Once the beta is released, following should work, I think. I haven't tested this at all.

I also added PyFrame_GetLasti() to the pythoncapi-compat project API, so it's available on Python 2.7-3.10: https://github.com/python/pythoncapi_compat You can use pythoncapi_compat.h to get the function on all Python versions.

@vstinner
Copy link
Author

vstinner commented Apr 8, 2022

I wrote #1352 to use PyFrame_GetLasti() in coverage: replace MyFrame_lasti() with PyFrame_GetLasti() using pythoncapi_compat.h. Sadly this PR drops support for Python 3.11a1..3.11a7.

@vstinner
Copy link
Author

vstinner commented Apr 8, 2022

Sadly this PR drops support for Python 3.11a1..3.11a7.

And #1353 is an attempt to keep support for Python 3.11 alpha versions.

@vstinner
Copy link
Author

vstinner commented Apr 8, 2022

Since there are concerns about worse performance caused by the usage of PyObject_GetAttrString(), I close this PR in favor of #1353 which is the newly added public function PyFrame_GetLasti().

@vstinner vstinner closed this Apr 8, 2022
@vstinner vstinner deleted the lasti branch April 8, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants