Skip to content

build: MyFrame_lasti() uses PyObject_GetAttrString()#1331

Closed
vstinner wants to merge 2 commits into
coveragepy:masterfrom
vstinner:lasti
Closed

build: MyFrame_lasti() uses PyObject_GetAttrString()#1331
vstinner wants to merge 2 commits into
coveragepy:masterfrom
vstinner:lasti

Conversation

@vstinner

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

Comment thread coverage/ctracer/util.h
#endif
static inline Py_ssize_t MyFrame_lasti(PyFrameObject *frame)
{
PyObject *obj = PyObject_GetAttrString((PyObject*)frame, "f_lasti");

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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

jouve commented Apr 7, 2022

Copy link
Copy Markdown

f_lasti is now gone : python/cpython#32208

@vstinner

vstinner commented Apr 7, 2022

Copy link
Copy Markdown
Author

f_lasti is now gone : python/cpython#32208

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

@jouve

jouve commented Apr 7, 2022

Copy link
Copy Markdown

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

@jouve

jouve commented Apr 7, 2022

Copy link
Copy Markdown

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

nedbat commented Apr 7, 2022

Copy link
Copy Markdown
Member

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

jouve commented Apr 7, 2022

Copy link
Copy Markdown

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
Copy Markdown

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

vstinner commented Apr 8, 2022

Copy link
Copy Markdown
Author

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

vstinner commented Apr 8, 2022

Copy link
Copy Markdown
Author

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

vstinner commented Apr 8, 2022

Copy link
Copy Markdown
Author

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

vstinner commented Apr 8, 2022

Copy link
Copy Markdown
Author

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