Skip to content
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

bpo-40941: Unify implicit and explicit state in the frame and generator objects into a single value. #20803

Merged
merged 10 commits into from
Jul 17, 2020
33 changes: 28 additions & 5 deletions Include/cpython/frameobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,21 @@
# error "this header file must not be included directly"
#endif

/* These values are chosen so that the inline functions below all
* compare f_state to zero.
*/
enum _framestate {
FRAME_CREATED = -2,
FRAME_SUSPENDED = -1,
FRAME_EXECUTING = 0,
FRAME_RETURNED = 1,
FRAME_UNWINDING = 2,
FRAME_RAISED = 3,
FRAME_CLEARED = 4
};

typedef signed char PyFrameState;

typedef struct {
int b_type; /* what kind of block this is */
int b_handler; /* where to jump to find handler */
Expand All @@ -18,11 +33,8 @@ struct _frame {
PyObject *f_globals; /* global symbol table (PyDictObject) */
PyObject *f_locals; /* local symbol table (any mapping) */
PyObject **f_valuestack; /* points after the last local */
/* Next free slot in f_valuestack. Frame creation sets to f_valuestack.
Frame evaluation usually NULLs it, but a frame that yields sets it
to the current stack top. */
PyObject **f_stacktop;
PyObject *f_trace; /* Trace function */
int f_stackdepth; /* Depth of value stack */
char f_trace_lines; /* Emit per-line trace events? */
char f_trace_opcodes; /* Emit per-opcode trace events? */

Expand All @@ -37,11 +49,22 @@ struct _frame {
bytecode index. */
int f_lineno; /* Current line number */
int f_iblock; /* index in f_blockstack */
char f_executing; /* whether the frame is still executing */
PyFrameState f_state; /* What state the frame is in */
PyTryBlock f_blockstack[CO_MAXBLOCKS]; /* for try and loop blocks */
PyObject *f_localsplus[1]; /* locals+stack, dynamically sized */
};

static inline int _PyFrame_IsRunnable(struct _frame *f) {
return f->f_state < FRAME_EXECUTING;
}

static inline int _PyFrame_IsExecuting(struct _frame *f) {
return f->f_state == FRAME_EXECUTING;
}

static inline int _PyFrameHasCompleted(struct _frame *f) {
Copy link
Member

Choose a reason for hiding this comment

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

Just something to be vary about here...there was recently https://bugs.python.org/issue39542#msg372962 where a static inline function failed to inline on mac, albeit it was through a more complicated call chain.

Not sure if you have the capability to test this but it'd be nice if someone can benchmark this on mac since this is such a critical code path.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should trust the big three compilers to do their jobs without constantly verifying it.
Clang will inline this very simple function, probably even if we don't tell it to.

return f->f_state > FRAME_EXECUTING;
}

/* Standard object interface */

Expand Down
2 changes: 0 additions & 2 deletions Include/genobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ extern "C" {
PyObject_HEAD \
/* Note: gi_frame can be NULL if the generator is "finished" */ \
PyFrameObject *prefix##_frame; \
/* True if generator is being executed. */ \
char prefix##_running; \
/* The code object backing the generator */ \
PyObject *prefix##_code; \
/* List of weak reference. */ \
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_generators.py
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,7 @@ def b():
>>> i.gi_running = 42
Traceback (most recent call last):
...
AttributeError: readonly attribute
AttributeError: attribute 'gi_running' of 'generator' objects is not writable
>>> def g():
... yield me.gi_running
>>> me = g()
Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_sys.py
Original file line number Diff line number Diff line change
Expand Up @@ -1236,7 +1236,7 @@ class C(object): pass
nfrees = len(x.f_code.co_freevars)
extras = x.f_code.co_stacksize + x.f_code.co_nlocals +\
ncells + nfrees - 1
check(x, vsize('5P2c4P3ic' + CO_MAXBLOCKS*'3i' + 'P' + extras*'P'))
check(x, vsize('4Pi2c4P3ic' + CO_MAXBLOCKS*'3i' + 'P' + extras*'P'))
# function
def func(): pass
check(func, size('13P'))
Expand All @@ -1253,7 +1253,7 @@ def bar(cls):
check(bar, size('PP'))
# generator
def get_gen(): yield 1
check(get_gen(), size('Pb2PPP4P'))
check(get_gen(), size('P2PPP4P'))
# iterator
check(iter('abc'), size('lP'))
# callable-iterator
Expand Down
3 changes: 3 additions & 0 deletions Lib/test/test_yield_from.py
Original file line number Diff line number Diff line change
Expand Up @@ -938,6 +938,9 @@ def two():
res.append(g1.throw(MyErr))
except StopIteration:
pass
except:
self.assertEqual(res, [0, 1, 2, 3])
raise
# Check with close
class MyIt(object):
def __iter__(self):
Expand Down
2 changes: 1 addition & 1 deletion Modules/_xxsubinterpretersmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1847,7 +1847,7 @@ _is_running(PyInterpreterState *interp)
return 0;
}

int executing = (int)(frame->f_executing);
int executing = _PyFrame_IsExecuting(frame);
Py_DECREF(frame);

return executing;
Expand Down
90 changes: 45 additions & 45 deletions Objects/frameobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -300,17 +300,20 @@ first_line_not_before(int *lines, int len, int line)
static void
frame_stack_pop(PyFrameObject *f)
{
PyObject *v = (*--f->f_stacktop);
assert(f->f_stackdepth >= 0);
f->f_stackdepth--;
PyObject *v = f->f_valuestack[f->f_stackdepth];
Py_DECREF(v);
}

static void
frame_block_unwind(PyFrameObject *f)
{
assert(f->f_stackdepth >= 0);
assert(f->f_iblock > 0);
f->f_iblock--;
PyTryBlock *b = &f->f_blockstack[f->f_iblock];
intptr_t delta = (f->f_stacktop - f->f_valuestack) - b->b_level;
intptr_t delta = f->f_stackdepth - b->b_level;
while (delta > 0) {
frame_stack_pop(f);
delta--;
Expand Down Expand Up @@ -352,33 +355,36 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore
return -1;
}

/* Upon the 'call' trace event of a new frame, f->f_lasti is -1 and
* f->f_trace is NULL, check first on the first condition.
* Forbidding jumps from the 'call' event of a new frame is a side effect
* of allowing to set f_lineno only from trace functions. */
if (f->f_lasti == -1) {
PyErr_Format(PyExc_ValueError,
/*
* This code preserves the historical restrictions on
* setting the line number of a frame.
* Jumps are forbidden on a 'return' trace event (except after a yield).
* Jumps from 'call' trace events are also forbidden.
* In addition, jumps are forbidden when not tracing,
* as this is a debugging feature.
*/
switch(f->f_state) {
case FRAME_CREATED:
PyErr_Format(PyExc_ValueError,
"can't jump from the 'call' trace event of a new frame");
return -1;
}

/* You can only do this from within a trace function, not via
* _getframe or similar hackery. */
if (!f->f_trace) {
PyErr_Format(PyExc_ValueError,
"f_lineno can only be set by a trace function");
return -1;
}

/* Forbid jumps upon a 'return' trace event (except after executing a
* YIELD_VALUE or YIELD_FROM opcode, f_stacktop is not NULL in that case)
* and upon an 'exception' trace event.
* Jumps from 'call' trace events have already been forbidden above for new
* frames, so this check does not change anything for 'call' events. */
if (f->f_stacktop == NULL) {
PyErr_SetString(PyExc_ValueError,
return -1;
case FRAME_RETURNED:
case FRAME_UNWINDING:
case FRAME_RAISED:
case FRAME_CLEARED:
PyErr_SetString(PyExc_ValueError,
"can only jump from a 'line' trace event");
return -1;
return -1;

Choose a reason for hiding this comment

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

Is a "call" trace also a "line" trace? If not, it would make sense to combine the 4 invalid states into a single error message, and it might make sense to include the invalid state as an exception attribute, or even as part of the string. if that is much easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

No a "call" trace is not a "line" trace.

case FRAME_EXECUTING:
case FRAME_SUSPENDED:
/* You can only do this from within a trace function, not via
* _getframe or similar hackery. */

Choose a reason for hiding this comment

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

Why this restriction? Is it just because "better safe than sorry, and we couldn't add the restriction later"? Is it an intentional limitation to be kind to PyPy and optimization?

Copy link
Member Author

Choose a reason for hiding this comment

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

It a pre-existing restriction. There is no change to behavior here.

if (!f->f_trace) {
PyErr_Format(PyExc_ValueError,
"f_lineno can only be set by a trace function");
return -1;
}
break;
}

int new_lineno;
Expand Down Expand Up @@ -585,11 +591,10 @@ frame_dealloc(PyFrameObject *f)
}

/* Free stack */
if (f->f_stacktop != NULL) {
for (PyObject **p = valuestack; p < f->f_stacktop; p++) {
Py_XDECREF(*p);
}
for (int i = 0; i < f->f_stackdepth; i++) {
Py_XDECREF(f->f_valuestack[i]);
}
f->f_stackdepth = 0;

Choose a reason for hiding this comment

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

I almost want to change this to take advantage of f_stackdepth already being an integer count of the number of times to loop, but maybe that is too clever for debugging, and should be left to the compiler.

while (f->f_stackdepth--) Py_XDRECREF(f->f_valuestack[f->f_stackdepth]) ;

seems not worse, but if I missed something by not testing, that argues for not making similar changes. :{

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should leave optimizing the loop to the compiler, although using idiomatic forms usually helps the compiler.
The two forms are equivalent.
The standard for loop from 0 to the limit is idiomatic C and easier to read.


Py_XDECREF(f->f_back);
Py_DECREF(f->f_builtins);
Expand Down Expand Up @@ -647,10 +652,8 @@ frame_traverse(PyFrameObject *f, visitproc visit, void *arg)
}

/* stack */
if (f->f_stacktop != NULL) {
for (PyObject **p = f->f_valuestack; p < f->f_stacktop; p++) {
Py_VISIT(*p);
}
for (int i = 0; i < f->f_stackdepth; i++) {
Py_VISIT(f->f_valuestack[i]);
}
return 0;
}
Expand All @@ -663,9 +666,7 @@ frame_tp_clear(PyFrameObject *f)
* frame may also point to this frame, believe itself to still be
* active, and try cleaning up this frame again.
*/
PyObject **oldtop = f->f_stacktop;
f->f_stacktop = NULL;
f->f_executing = 0;
f->f_state = FRAME_CLEARED;

Py_CLEAR(f->f_trace);

Expand All @@ -676,18 +677,17 @@ frame_tp_clear(PyFrameObject *f)
}

/* stack */
if (oldtop != NULL) {
for (PyObject **p = f->f_valuestack; p < oldtop; p++) {
Py_CLEAR(*p);
}
for (int i = 0; i < f->f_stackdepth; i++) {
Py_CLEAR(f->f_valuestack[i]);
}
f->f_stackdepth = 0;
return 0;
}

static PyObject *
frame_clear(PyFrameObject *f, PyObject *Py_UNUSED(ignored))
{
if (f->f_executing) {
if (_PyFrame_IsExecuting(f)) {
PyErr_SetString(PyExc_RuntimeError,
"cannot clear an executing frame");
return NULL;
Expand Down Expand Up @@ -898,7 +898,7 @@ _PyFrame_New_NoTrack(PyThreadState *tstate, PyCodeObject *code,
return NULL;
}

f->f_stacktop = f->f_valuestack;
f->f_stackdepth = 0;
f->f_builtins = builtins;
Py_XINCREF(back);
f->f_back = back;
Expand Down Expand Up @@ -927,7 +927,7 @@ _PyFrame_New_NoTrack(PyThreadState *tstate, PyCodeObject *code,
f->f_lasti = -1;
f->f_lineno = code->co_firstlineno;
f->f_iblock = 0;
f->f_executing = 0;
f->f_state = FRAME_CREATED;
f->f_gen = NULL;
f->f_trace_opcodes = 0;
f->f_trace_lines = 1;
Expand Down
Loading