Skip to content

bpo-45256: Rationalize code around Python-to-Python calls a bit. #29235

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

Merged
merged 2 commits into from
Oct 28, 2021
Merged

bpo-45256: Rationalize code around Python-to-Python calls a bit. #29235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9cc3f1f
Tighten up Python-to-Python calls.
markshannon Oct 26, 2021
0a93c9f
Rename a label for clarity.
markshannon Oct 28, 2021
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
15 changes: 15 additions & 0 deletions Include/internal/pycore_frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,21 @@ _PyFrame_LocalsToFast(InterpreterFrame *frame, int clear);
InterpreterFrame *_PyThreadState_PushFrame(
PyThreadState *tstate, PyFrameConstructor *con, PyObject *locals);

extern InterpreterFrame *
_PyThreadState_BumpFramePointerSlow(PyThreadState *tstate, size_t size);

static inline InterpreterFrame *
_PyThreadState_BumpFramePointer(PyThreadState *tstate, size_t size)
{
PyObject **base = tstate->datastack_top;
PyObject **top = base + size;
if (top < tstate->datastack_limit) {
tstate->datastack_top = top;
return (InterpreterFrame *)base;
}
return _PyThreadState_BumpFramePointerSlow(tstate, size);
}

void _PyThreadState_PopFrame(PyThreadState *tstate, InterpreterFrame *frame);

#ifdef __cplusplus
Expand Down
11 changes: 5 additions & 6 deletions Objects/frameobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,16 +786,15 @@ allocate_heap_frame(PyFrameConstructor *con, PyObject *locals)
{
PyCodeObject *code = (PyCodeObject *)con->fc_code;
int size = code->co_nlocalsplus+code->co_stacksize + FRAME_SPECIALS_SIZE;
PyObject **localsarray = PyMem_Malloc(sizeof(PyObject *)*size);
if (localsarray == NULL) {
InterpreterFrame *frame = (InterpreterFrame *)PyMem_Malloc(sizeof(PyObject *)*size);
if (frame == NULL) {
PyErr_NoMemory();
return NULL;
}
for (Py_ssize_t i=0; i < code->co_nlocalsplus; i++) {
localsarray[i] = NULL;
}
InterpreterFrame *frame = (InterpreterFrame *)(localsarray + code->co_nlocalsplus);
_PyFrame_InitializeSpecials(frame, con, locals, code->co_nlocalsplus);
for (Py_ssize_t i = 0; i < code->co_nlocalsplus; i++) {
frame->localsplus[i] = NULL;
}
return frame;
}

Expand Down
141 changes: 84 additions & 57 deletions Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ static int get_exception_handler(PyCodeObject *, int, int*, int*, int*);
static InterpreterFrame *
_PyEvalFramePushAndInit(PyThreadState *tstate, PyFrameConstructor *con,
PyObject *locals, PyObject* const* args,
size_t argcount, PyObject *kwnames, int steal_args);
size_t argcount, PyObject *kwnames);
static int
_PyEvalFrameClearAndPop(PyThreadState *tstate, InterpreterFrame * frame);

Expand Down Expand Up @@ -4646,7 +4646,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
InterpreterFrame *new_frame = _PyEvalFramePushAndInit(
tstate, PyFunction_AS_FRAME_CONSTRUCTOR(function), locals,
stack_pointer,
nargs, kwnames, 1);
nargs, kwnames);
STACK_SHRINK(postcall_shrink);
// The frame has stolen all the arguments from the stack,
// so there is no need to clean them up.
Expand Down Expand Up @@ -4720,11 +4720,14 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
/* PEP 523 */
DEOPT_IF(tstate->interp->eval_frame != NULL, CALL_FUNCTION);
STAT_INC(CALL_FUNCTION, hit);
InterpreterFrame *new_frame = _PyThreadState_PushFrame(
tstate, PyFunction_AS_FRAME_CONSTRUCTOR(func), NULL);
PyCodeObject *code = (PyCodeObject *)func->func_code;
size_t size = code->co_nlocalsplus + code->co_stacksize + FRAME_SPECIALS_SIZE;
InterpreterFrame *new_frame = _PyThreadState_BumpFramePointer(tstate, size);
if (new_frame == NULL) {
goto error;
}
_PyFrame_InitializeSpecials(new_frame, PyFunction_AS_FRAME_CONSTRUCTOR(func),
NULL, code->co_nlocalsplus);
STACK_SHRINK(argcount);
for (int i = 0; i < argcount; i++) {
new_frame->localsplus[i] = stack_pointer[i];
Expand All @@ -4735,6 +4738,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
Py_INCREF(def);
new_frame->localsplus[argcount+i] = def;
}
for (int i = argcount+deflen; i < code->co_nlocalsplus; i++) {
new_frame->localsplus[i] = NULL;
}
STACK_SHRINK(1);
Py_DECREF(func);
_PyFrame_SetStackPointer(frame, stack_pointer);
Expand Down Expand Up @@ -5592,7 +5598,7 @@ get_exception_handler(PyCodeObject *code, int index, int *level, int *handler, i
static int
initialize_locals(PyThreadState *tstate, PyFrameConstructor *con,
PyObject **localsplus, PyObject *const *args,
Py_ssize_t argcount, PyObject *kwnames, int steal_args)
Py_ssize_t argcount, PyObject *kwnames)
{
PyCodeObject *co = (PyCodeObject*)con->fc_code;
const Py_ssize_t total_args = co->co_argcount + co->co_kwonlyargcount;
Expand Down Expand Up @@ -5626,21 +5632,14 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con,
}
for (j = 0; j < n; j++) {
PyObject *x = args[j];
if (!steal_args) {
Py_INCREF(x);
}
assert(localsplus[j] == NULL);
localsplus[j] = x;
}

/* Pack other positional arguments into the *args argument */
if (co->co_flags & CO_VARARGS) {
PyObject *u = NULL;
if (steal_args) {
u = _PyTuple_FromArraySteal(args + n, argcount - n);
} else {
u = _PyTuple_FromArray(args + n, argcount - n);
}
u = _PyTuple_FromArraySteal(args + n, argcount - n);
if (u == NULL) {
goto fail_post_positional;
}
Expand All @@ -5649,10 +5648,8 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con,
}
else if (argcount > n) {
/* Too many postional args. Error is reported later */
if (steal_args) {
for (j = n; j < argcount; j++) {
Py_DECREF(args[j]);
}
for (j = n; j < argcount; j++) {
Py_DECREF(args[j]);
}
}

Expand Down Expand Up @@ -5714,19 +5711,15 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con,
if (PyDict_SetItem(kwdict, keyword, value) == -1) {
goto kw_fail;
}
if (steal_args) {
Py_DECREF(value);
}
Py_DECREF(value);
continue;

kw_fail:
if (steal_args) {
for (;i < kwcount; i++) {
PyObject *value = args[i+argcount];
Py_DECREF(value);
}
for (;i < kwcount; i++) {
PyObject *value = args[i+argcount];
Py_DECREF(value);
}
goto fail_noclean;
goto fail_post_args;

kw_found:
if (localsplus[j] != NULL) {
Expand All @@ -5735,9 +5728,6 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con,
con->fc_qualname, keyword);
goto kw_fail;
}
if (!steal_args) {
Py_INCREF(value);
}
localsplus[j] = value;
}
}
Expand All @@ -5746,7 +5736,7 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con,
if ((argcount > co->co_argcount) && !(co->co_flags & CO_VARARGS)) {
too_many_positional(tstate, co, argcount, con->fc_defaults, localsplus,
con->fc_qualname);
goto fail_noclean;
goto fail_post_args;
}

/* Add missing positional arguments (copy default values from defs) */
Expand All @@ -5762,7 +5752,7 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con,
if (missing) {
missing_arguments(tstate, co, missing, defcount, localsplus,
con->fc_qualname);
goto fail_noclean;
goto fail_post_args;
}
if (n > m)
i = n - m;
Expand Down Expand Up @@ -5795,43 +5785,39 @@ initialize_locals(PyThreadState *tstate, PyFrameConstructor *con,
continue;
}
else if (_PyErr_Occurred(tstate)) {
goto fail_noclean;
goto fail_post_args;
}
}
missing++;
}
if (missing) {
missing_arguments(tstate, co, missing, -1, localsplus,
con->fc_qualname);
goto fail_noclean;
goto fail_post_args;
}
}

/* Copy closure variables to free variables */
for (i = 0; i < co->co_nfreevars; ++i) {
PyObject *o = PyTuple_GET_ITEM(con->fc_closure, i);
Py_INCREF(o);
localsplus[co->co_nlocals + co->co_nplaincellvars + i] = o;
}

return 0;

fail_pre_positional:
if (steal_args) {
for (j = 0; j < argcount; j++) {
Py_DECREF(args[j]);
}
for (j = 0; j < argcount; j++) {
Py_DECREF(args[j]);
}
/* fall through */
fail_post_positional:
if (steal_args) {
Py_ssize_t kwcount = kwnames != NULL ? PyTuple_GET_SIZE(kwnames) : 0;
if (kwnames) {
Py_ssize_t kwcount = PyTuple_GET_SIZE(kwnames);
for (j = argcount; j < argcount+kwcount; j++) {
Py_DECREF(args[j]);
}
}
/* fall through */
fail_noclean:
fail_post_args:
return -1;
}

Expand All @@ -5847,21 +5833,34 @@ make_coro_frame(PyThreadState *tstate,
int size = code->co_nlocalsplus+code->co_stacksize + FRAME_SPECIALS_SIZE;
InterpreterFrame *frame = (InterpreterFrame *)PyMem_Malloc(sizeof(PyObject *)*size);
if (frame == NULL) {
PyErr_NoMemory();
return NULL;
goto fail;
}
for (Py_ssize_t i=0; i < code->co_nlocalsplus; i++) {
_PyFrame_InitializeSpecials(frame, con, locals, code->co_nlocalsplus);
for (int i = 0; i < code->co_nlocalsplus; i++) {
frame->localsplus[i] = NULL;
}
_PyFrame_InitializeSpecials(frame, con, locals, code->co_nlocalsplus);
assert(frame->frame_obj == NULL);
if (initialize_locals(tstate, con, frame->localsplus, args, argcount, kwnames, 0)) {
if (initialize_locals(tstate, con, frame->localsplus, args, argcount, kwnames)) {
_PyFrame_Clear(frame, 1);
return NULL;
}
return frame;
fail:
Copy link
Member

Choose a reason for hiding this comment

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

Why this tag? There is only one place where you can reach the tag, no? Is also not future proof, because you are only raising PyErr_NoMemory so I am not sure what is the advantage. Is for separating error handling from the logic?

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 is just to keep the error handling out of the main block of code.
It is a quite common idiom in CPython to put the error handling at the end. Personally, I think early exits help readability, so I think it is worth doing even if there is only a single jump to the error handler.

I see you point about PyErr_NoMemory though, I'll rename the label to fail_no_memory for clarity.

/* Consume the references */
for (Py_ssize_t i = 0; i < argcount; i++) {
Py_DECREF(args[i]);
}
if (kwnames) {
Py_ssize_t kwcount = PyTuple_GET_SIZE(kwnames);
for (Py_ssize_t i = 0; i < kwcount; i++) {
Py_DECREF(args[i+argcount]);
}
}
PyErr_NoMemory();
return NULL;
}

/* Consumes all the references to the args */
static PyObject *
make_coro(PyThreadState *tstate, PyFrameConstructor *con,
PyObject *locals,
Expand All @@ -5877,30 +5876,46 @@ make_coro(PyThreadState *tstate, PyFrameConstructor *con,
if (gen == NULL) {
return NULL;
}

return gen;
}

// If *steal_args* is set, the function will steal the references to all the arguments.
// In case of error, the function returns null and if *steal_args* is set, the caller
// will still own all the arguments.
/* Consumes all the references to the args */
static InterpreterFrame *
_PyEvalFramePushAndInit(PyThreadState *tstate, PyFrameConstructor *con,
PyObject *locals, PyObject* const* args,
size_t argcount, PyObject *kwnames, int steal_args)
size_t argcount, PyObject *kwnames)
{
InterpreterFrame * frame = _PyThreadState_PushFrame(tstate, con, locals);
PyCodeObject * code = (PyCodeObject *)con->fc_code;
size_t size = code->co_nlocalsplus + code->co_stacksize + FRAME_SPECIALS_SIZE;
InterpreterFrame *frame = _PyThreadState_BumpFramePointer(tstate, size);
if (frame == NULL) {
return NULL;
goto fail;
}
PyObject **localsarray = _PyFrame_GetLocalsArray(frame);
if (initialize_locals(tstate, con, localsarray, args, argcount, kwnames, steal_args)) {
_PyFrame_InitializeSpecials(frame, con, locals, code->co_nlocalsplus);
PyObject **localsarray = &frame->localsplus[0];
for (int i = 0; i < code->co_nlocalsplus; i++) {
localsarray[i] = NULL;
}
if (initialize_locals(tstate, con, localsarray, args, argcount, kwnames)) {
_PyFrame_Clear(frame, 0);
return NULL;
}
frame->previous = tstate->frame;
tstate->frame = frame;
return frame;
fail:
Copy link
Member

Choose a reason for hiding this comment

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

Question: If the call succeeds, don't we need to do this work anyway in initialize_locals? Not sure if is going to be more messy, but we could maybe share this path or part of this path for error and not error cases. For instance, remove these parts in initialize_locals:

    if (kwnames) {
        Py_ssize_t kwcount = PyTuple_GET_SIZE(kwnames);
        for (j = argcount; j < argcount+kwcount; j++) {
            Py_DECREF(args[j]);
        }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. If the call succeeds all the argument references are consumed by moving them into the local variable array.

/* Consume the references */
for (size_t i = 0; i < argcount; i++) {
Py_DECREF(args[i]);
}
if (kwnames) {
Py_ssize_t kwcount = PyTuple_GET_SIZE(kwnames);
for (Py_ssize_t i = 0; i < kwcount; i++) {
Py_DECREF(args[i+argcount]);
}
}
PyErr_NoMemory();
return NULL;
}

static int
Expand All @@ -5925,13 +5940,25 @@ _PyEval_Vector(PyThreadState *tstate, PyFrameConstructor *con,
PyObject *kwnames)
{
PyCodeObject *code = (PyCodeObject *)con->fc_code;
/* _PyEvalFramePushAndInit and make_coro consume
* all the references to their arguments
*/
for (size_t i = 0; i < argcount; i++) {
Py_INCREF(args[i]);
}
if (kwnames) {
Py_ssize_t kwcount = PyTuple_GET_SIZE(kwnames);
for (Py_ssize_t i = 0; i < kwcount; i++) {
Py_INCREF(args[i+argcount]);
}
}
int is_coro = code->co_flags &
(CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR);
if (is_coro) {
return make_coro(tstate, con, locals, args, argcount, kwnames);
}
InterpreterFrame *frame = _PyEvalFramePushAndInit(
tstate, con, locals, args, argcount, kwnames, 0);
tstate, con, locals, args, argcount, kwnames);
if (frame == NULL) {
return NULL;
}
Expand Down
22 changes: 16 additions & 6 deletions Python/pystate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2065,12 +2065,8 @@ push_chunk(PyThreadState *tstate, int size)
}

InterpreterFrame *
_PyThreadState_PushFrame(PyThreadState *tstate, PyFrameConstructor *con, PyObject *locals)
_PyThreadState_BumpFramePointerSlow(PyThreadState *tstate, size_t size)
{
PyCodeObject *code = (PyCodeObject *)con->fc_code;
int nlocalsplus = code->co_nlocalsplus;
size_t size = nlocalsplus + code->co_stacksize +
FRAME_SPECIALS_SIZE;
assert(size < INT_MAX/sizeof(PyObject *));
PyObject **base = tstate->datastack_top;
PyObject **top = base + size;
Expand All @@ -2083,7 +2079,21 @@ _PyThreadState_PushFrame(PyThreadState *tstate, PyFrameConstructor *con, PyObjec
else {
tstate->datastack_top = top;
}
InterpreterFrame *frame = (InterpreterFrame *)base;
return (InterpreterFrame *)base;
}


InterpreterFrame *
_PyThreadState_PushFrame(PyThreadState *tstate, PyFrameConstructor *con, PyObject *locals)
{
PyCodeObject *code = (PyCodeObject *)con->fc_code;
int nlocalsplus = code->co_nlocalsplus;
size_t size = nlocalsplus + code->co_stacksize +
FRAME_SPECIALS_SIZE;
InterpreterFrame *frame = _PyThreadState_BumpFramePointer(tstate, size);
if (frame == NULL) {
return NULL;
}
_PyFrame_InitializeSpecials(frame, con, locals, nlocalsplus);
for (int i=0; i < nlocalsplus; i++) {
frame->localsplus[i] = NULL;
Expand Down