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

markshannon · 2020-06-11T13:27:40Z

Merges the explicit state in PyFrameObject.f_excuting and PyGenObject.gi_running, and the implicit state in PyFrameObject.f_stacktop == NULL and PyFrameObject.f_last == -1 into a single enum field in PyFrameObject

Since we no longer need to test PyFrameObject.f_stacktop == NULL , The f_stacktop pointer can be replaced with a f_stackdepth integer, which makes for simpler code when iterating over the stack and avoids the potential hazard of NULL pointers.

There are three benefits to these changes:

The code is more robust and, IMO, easier to understand, as all state is now explicit.
It carries additional information about the state of the frame. Information about whether a frame exiting by return or by raise is available.
A modest reduction in size of frame and generator objects.

https://bugs.python.org/issue40941

bedevere-bot · 2020-06-11T14:42:48Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 867427e269564d3b16b7c5580343556cbfdcad7c 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

JimJJewett · 2020-07-09T18:03:14Z

Include/cpython/frameobject.h

+    FRAME_RETURNED = 1,
+    FRAME_RAISED = 2,
+    FRAME_CLEARED = 3
+} PyFrameState;


Doesn't this default to the size of an int, rather than a char? I'm not yet sure whether the previous explicit sizing on some variables was actually useful, or just lost to alignment, but it seems like a signed char should still be more than enough.

I've changed it to a signed char

JimJJewett · 2020-07-09T18:32:23Z

Objects/frameobject.c

    /* 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)
+     * YIELD_VALUE or YIELD_FROM opcode)
     * 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. */


Does "above" refer to the code just deleted, and no longer visible to future readers? Can this comment be reworded to something with fewer separate branching conditions, such as "Jumps can only be made via a trace function triggered by a line trace event on a currently executing or suspended frame" I am assuming that this is not actually a new restriction that needs to be documented, but that is an assumption; I won't be shocked if I missed something outside the diff.

JimJJewett · 2020-07-09T18:34:34Z

Objects/frameobject.c

                "can only jump from a 'line' trace event");
-        return -1;
+            return -1;


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.

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

JimJJewett · 2020-07-09T18:37:17Z

Objects/frameobject.c

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


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?

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

JimJJewett · 2020-07-09T18:49:00Z

Objects/frameobject.c

    }
+    f->f_stackdepth = 0;


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. :{

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.

JimJJewett · 2020-07-09T19:24:15Z

Objects/genobject.c

            err = gen_close_iter(yf);
-            gen->gi_running = 0;
+            gen->gi_frame->f_state = state;


If the previous state was not FRAME_SUSPENDED, what would that mean, and should the generator really return to that state? (applies several places) I had sort of thought that the only backwards transitions were FRAME_EXECUTING to FRAME_SUSPENDED, and FRAME_CLEARED to FRAME_CREATED when the memory was reallocated to a different frame.

I also think that the state must be FRAME_SUSPENDED, but if were some other state were possible this code would still be correct. So I'm inclined to leave it as is.

JimJJewett · 2020-07-09T19:32:33Z

Objects/genobject.c

 static PyGetSetDef gen_getsetlist[] = {
    {"__name__", (getter)gen_get_name, (setter)gen_set_name,
     PyDoc_STR("name of the generator")},
    {"__qualname__", (getter)gen_get_qualname, (setter)gen_set_qualname,
     PyDoc_STR("qualified name of the generator")},
    {"gi_yieldfrom", (getter)gen_getyieldfrom, NULL,
     PyDoc_STR("object being iterated by yield from, or None")},
+    {"gi_running", (getter)gen_getrunning, NULL, NULL},


Does this reordering break any binary guarantees for any of the types?

No. This is opaque to C extensions.

JimJJewett · 2020-07-09T19:42:31Z

Python/ceval.c

+       Having f_stackdepth <= 0 ensures that invalid
+       values are not visible to the cycle GC.
+       We choose -1 rather than 0 to assist debugging.
+     */


The GC doesn't seem to have been modified; does it really pay attention to stackdepth, as opposed to the actual pointer that was there previously?

JimJJewett · 2020-07-09T19:46:43Z

Python/ceval.c

+        if (tstate->c_tracefunc != NULL) {
+            /* Temporarily set frame state to RAISED for benefit of frame.setlineno */
+            assert(f->f_state == FRAME_EXECUTING);
+            f->f_state = FRAME_RAISED;


It seems I missed another backwards state transition. Maybe it would be good to explain the state machine where the states names are defined.

This is a bit of a hack, to keep frame.setlineno happy.
I've added FRAME_UNWINDING to mark that an exception is being unwound in the frame, but that the frame has not completed yet.

zooba · 2020-07-09T19:30:16Z

Include/cpython/frameobject.h

@@ -4,6 +4,16 @@
 #  error "this header file must not be included directly"
 #endif

+/* These values are chosen so that all tests involve comparing to zero. */


"all" tests?

I think this means ">0 implies finished; <=0 implies not-yet-finished", but I'm inferring that from the enum values and not the comment.

+1, making the specification explicit would be nice.

zooba · 2020-07-09T19:34:13Z

Include/cpython/frameobject.h

    PyTryBlock f_blockstack[CO_MAXBLOCKS]; /* for try and loop blocks */
    PyObject *f_localsplus[1];  /* locals+stack, dynamically sized */
 };

+static inline int _PyFrameIsRunnable(struct _frame *f) {


_PyFrame_IsRunnable?

Or maybe _PyFrame_IS_RUNNABLE?

+1 to _PyFrame_IsRunnable. I think a similar format should be used with the other functions as well, that naming style fits better with existing members.

_PyFrame_IsRunnable seem more in keeping with the rest of the code base. So I've gone with that.

aeros · 2020-07-10T03:42:43Z

Objects/genobject.c

-    if (f && f->f_stacktop) {
+    if (f) {


Would it make sense to replace the && f->f_stacktop with && f->f_stackdepth >=0 or at least an assert(f->f_stackdepth >= 0) on line 329?

Admittedly, I'm a bit out of my depth when it come to the low-level generator details, but at a glance it seems to me like we would not want to start the yield from if the generator's frame has a stack depth of -1 (or lower, somehow?).

In theory, the stack depth can never be negative. There's an assertion on line 344 that it is positive.

Yeah, I saw that assertion on line 344. If it's realistically not going to be negative, we probably only need the assertion prior to modifying the stack depth (particularly for decrements).

aeros · 2020-07-10T03:53:05Z

Objects/genobject.c

            /* Pop subiterator from stack */
-            ret = *(--gen->gi_frame->f_stacktop);
+            gen->gi_frame->f_stackdepth--;


Should we have an assert(gen->gi_frame->f_stackdepth >= 0) sanity check here after decrementing the stack depth? I don't know that it's necessary here, but since it gets removed with -O anyways it won't incur a real cost.

Note to self: assert(gen->gi_frame->f_stackdepth > 0); was added before decrement in a22412c.

ammaraskar · 2020-07-10T07:18:21Z

Include/cpython/frameobject.h

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


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.

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.

ammaraskar · 2020-07-10T07:18:54Z

Include/cpython/frameobject.h

@@ -4,6 +4,16 @@
 #  error "this header file must not be included directly"
 #endif

+/* These values are chosen so that all tests involve comparing to zero. */


+1, making the specification explicit would be nice.

ammaraskar · 2020-07-10T07:20:58Z

Objects/genobject.c

+gen_getrunning(PyGenObject *gen, void *Py_UNUSED(ignored))
+{
+    if (gen->gi_frame == NULL) {
+        Py_INCREF(Py_False);


nit: Py_RETURN_FALSE

ammaraskar · 2020-07-10T07:21:18Z

Objects/genobject.c

+cr_getrunning(PyCoroObject *coro, void *Py_UNUSED(ignored))
+{
+    if (coro->cr_frame == NULL) {
+        Py_INCREF(Py_False);


nit: Py_RETURN_FALSE

…to read and saves a word of memory.

…ility.

markshannon · 2020-07-14T16:14:52Z

Thanks for the reviews. I think I've addressed all the comments.

cjerdonek · 2020-07-14T23:09:50Z

Objects/genobject.c

            ret = _gen_throw((PyGenObject *)yf, close_on_genexit,
                             typ, val, tb);
            tstate->frame = f;
-            gen->gi_running = 0;
+            gen->gi_frame->f_state = state;


Nit: for symmetry, I think the new modified line would be better before the tstate->frame = f; line.

…or objects into a single value. (pythonGH-20803) * Merge gen and frame state variables into one. * Replace stack pointer with depth in PyFrameObject. Makes code easier to read and saves a word of memory.

markshannon requested a review from 1st1 as a code owner June 11, 2020 13:27

the-knights-who-say-ni added the CLA signed label Jun 11, 2020

bedevere-bot added the awaiting core review label Jun 11, 2020

markshannon added the skip news label Jun 11, 2020

markshannon changed the title ~~bpo-40941: Unify implicit and explicit the state in frame and generator into a single value.~~ bpo-40941: Unify implicit and explicit state in the frame and generator objects into a single value. Jun 11, 2020

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 11, 2020

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 11, 2020

serhiy-storchaka self-requested a review July 9, 2020 10:20

pablogsal self-requested a review July 9, 2020 12:06

JimJJewett reviewed Jul 9, 2020

View reviewed changes

zooba reviewed Jul 9, 2020

View reviewed changes

aeros requested a review from njsmith July 10, 2020 02:25

aeros reviewed Jul 10, 2020

View reviewed changes

ammaraskar reviewed Jul 10, 2020

View reviewed changes

markshannon added 9 commits July 14, 2020 16:45

Merge gen and frame state variables into one.

0d4d7e4

Replace stack pointer with depth in PyFrameObject. Makes code easier …

a7e7f75

…to read and saves a word of memory.

Tweak handling of f_stackdepth.

fe00513

Change _PyFrameIsExecuting to _PyFrame_IsExecuting for improved legib…

eeb0e4d

…ility.

Reword comment in frame_setlineno for clarity.

286f281

Fix messed up replace.

39d0996

Change f_state to use a single byte.

15a965e

minor tidy ups to genobject.c

a22412c

Add FRAME_UNWINDING to mark unwinding state.

38792f2

markshannon force-pushed the merge-frame-gen-state branch from 593d935 to 38792f2 Compare July 14, 2020 16:14

cjerdonek reviewed Jul 14, 2020

View reviewed changes

Make state changes around call to _gen_throw symmetrical.

21a8f2b

markshannon merged commit cb9879b into python:master Jul 17, 2020

bedevere-bot removed the awaiting core review label Jul 17, 2020

markshannon deleted the merge-frame-gen-state branch July 17, 2020 10:44

isidentical mentioned this pull request Oct 12, 2020

3.10 compatability -- changes to frame object sumerc/yappi#65

Closed

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

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

Conversation

markshannon commented Jun 11, 2020 • edited Loading

bedevere-bot commented Jun 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeros Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeros Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeros Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

aeros Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon commented Jun 11, 2020 •

edited

Loading

aeros Jul 10, 2020 •

edited

Loading

aeros Jul 10, 2020 •

edited

Loading

aeros Jul 10, 2020 •

edited

Loading

aeros Jul 15, 2020 •

edited

Loading