10000 build: MyFrame_lasti() uses PyObject_GetAttrString() by vstinner · Pull Request #1331 · nedbat/coveragepy · GitHub
[go: up one dir, main page]
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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a60e4be
build: MyFrame_lasti() uses PyObject_GetAttrString()
vstinner Feb 23, 2022
6094481
Fix Python 3.8 support
vstinner Feb 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions coverage/ctracer/tracer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,16 +525,18 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame)
*/
BOOL real_call = FALSE;

Py_ssize_t lasti = MyFrame_lasti(frame);
#ifdef RESUME // 3.11.0a4
/*
* The current opcode is guaranteed to be RESUME. The argument
* determines what kind of resume it is.
*/
PyObject * pCode = MyFrame_GetCode(frame)->co_code;
real_call = (PyBytes_AS_STRING(pCode)[MyFrame_lasti(frame) + 1] == 0);
assert(0 <= lasti >= 0 && (lasti + 1) < PyBytes_GET_SIZE(pCode));
real_call = (PyBytes_AS_STRING(pCode)[lasti + 1] == 0);
#else
// f_lasti is -1 for a true call, and a real byte offset for a generator re-entry.
real_call = (MyFrame_lasti(frame) < 0);
real_call = (lasti < 0);
#endif

if (real_call) {
Expand Down Expand Up @@ -699,11 +701,12 @@ CTracer_handle_return(CTracer *self, PyFrameObject *frame)
if (self->tracing_arcs && self->pcur_entry->file_data) {
BOOL real_return = FALSE;
PyObject * pCode = MyFrame_GetCode(frame)->co_code;
int lasti = MyFrame_lasti(frame);
Py_ssize_t lasti = MyFrame_lasti(frame);
Py_ssize_t code_size = PyBytes_GET_SIZE(pCode);
unsigned char * code_bytes = (unsigned char *)PyBytes_AS_STRING(pCode);
#ifdef RESUME
if (lasti == code_size - 2) {
assert(0 <= lasti);
if ((lasti + 2) == code_size) {
real_return = TRUE;
}
else {
Expand All @@ -718,6 +721,7 @@ CTracer_handle_return(CTracer *self, PyFrameObject *frame)
BOOL is_yield = FALSE;
BOOL is_yield_from = FALSE;

assert(0 <= lasti);
if (lasti < code_size) {
is_yield = (code_bytes[lasti] == YIELD_VALUE);
if (lasti + 2 < code_size) {
Expand Down
29 changes: 16 additions & 13 deletions coverage/ctracer/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,29 @@
#define _COVERAGE_UTIL_H

#include <Python.h>
#include "frameobject.h" // PyFrameObject

/* Compile-time debugging helpers */
#undef WHAT_LOG /* Define to log the WHAT params in the trace function. */
#undef TRACE_LOG /* Define to log our bookkeeping. */
#undef COLLECT_STATS /* Collect counters: stats are printed when tracer is stopped. */
#undef DO_NOTHING /* Define this to make the tracer do nothing. */

#if PY_VERSION_HEX >= 0x030B00A0
// 3.11 moved f_lasti into an internal structure. This is totally the wrong way
// to make this work, but it's all I've got until https://bugs.python.org/issue40421
// is resolved.
#include <internal/pycore_frame.h>
#define MyFrame_lasti(f) ((f)->f_frame->f_lasti * 2)
#elif PY_VERSION_HEX >= 0x030A00A7
// The f_lasti field changed meaning in 3.10.0a7. It had been bytes, but
// now is instructions, so we need to adjust it to use it as a byte index.
#define MyFrame_lasti(f) ((f)->f_lasti * 2)
#else
#define MyFrame_lasti(f) ((f)->f_lasti)
#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.

if (obj == NULL) {
PyErr_Clear();
return -1;
}
// f_lasti is an int, but using Py_ssize_t is more convenient in coverage
Py_ssize_t lasti = PyLong_AsSsize_t(obj);
if (lasti == -1 && PyErr_Occurred()) {
PyErr_Clear();
return -1;
}
return lasti;
}

// Access f_code should be done through a helper starting in 3.9.
#if PY_VERSION_HEX >= 0x03090000
Expand Down
0