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-45786: Allocate space for frame in frame object. #29729

Merged
merged 6 commits into from
Nov 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions Include/cpython/frameobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ struct _frame {
int f_lineno; /* Current line number. Only valid if non-zero */
char f_trace_lines; /* Emit per-line trace events? */
char f_trace_opcodes; /* Emit per-opcode trace events? */
char f_own_locals_memory; /* This frame owns the memory for the locals */
char f_owns_frame; /* This frame owns the frame */
/* The frame data, if this frame object owns the frame */
PyObject *_f_frame_data[1];
};

/* Standard object interface */
Expand All @@ -26,7 +28,7 @@ PyAPI_FUNC(PyFrameObject *) PyFrame_New(PyThreadState *, PyCodeObject *,

/* only internal use */
PyFrameObject*
_PyFrame_New_NoTrack(struct _interpreter_frame *, int);
_PyFrame_New_NoTrack(PyCodeObject *code);


/* The rest of the interface is specific for frame objects */
Expand Down
7 changes: 3 additions & 4 deletions Include/internal/pycore_frame.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ static inline void _PyFrame_StackPush(InterpreterFrame *f, PyObject *value) {

#define FRAME_SPECIALS_SIZE ((sizeof(InterpreterFrame)-1)/sizeof(PyObject *))

InterpreterFrame *
_PyInterpreterFrame_HeapAlloc(PyFunctionObject *func, PyObject *locals);
InterpreterFrame *_PyFrame_Copy(InterpreterFrame *frame);

static inline void
_PyFrame_InitializeSpecials(
Expand Down Expand Up @@ -139,8 +138,8 @@ _PyFrame_GetFrameObject(InterpreterFrame *frame)
* take should be set to 1 for heap allocated
* frames like the ones in generators and coroutines.
*/
int
_PyFrame_Clear(InterpreterFrame * frame, int take);
void
_PyFrame_Clear(InterpreterFrame * frame);

int
_PyFrame_Traverse(InterpreterFrame *frame, visitproc visit, void *arg);
Expand Down
1 change: 0 additions & 1 deletion Include/internal/pycore_gc.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ extern Py_ssize_t _PyGC_CollectNoFail(PyThreadState *tstate);


// Functions to clear types free lists
extern void _PyFrame_ClearFreeList(PyInterpreterState *interp);
extern void _PyTuple_ClearFreeList(PyInterpreterState *interp);
extern void _PyFloat_ClearFreeList(PyInterpreterState *interp);
extern void _PyList_ClearFreeList(PyInterpreterState *interp);
Expand Down
14 changes: 0 additions & 14 deletions Include/internal/pycore_interp.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ struct _Py_unicode_state {
# define PyTuple_MAXFREELIST 1
# define PyList_MAXFREELIST 0
# define PyDict_MAXFREELIST 0
# define PyFrame_MAXFREELIST 0
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing the freelist? The commit doesn't explain anything for this

Copy link
Member Author

Choose a reason for hiding this comment

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

The frameobjects are no longer a fixed size, so a free list wouldn't work.

We shouldn't be allocating many frameobjects anyway, they only get creating when an exception is raised, or explicitly requested.

Copy link
Member

Choose a reason for hiding this comment

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

But this will happen when tracing/profiling, no? I suspect coverage runs for example will be slower due to this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but freelists just don't work unless all objects are the same size.

Copy link
Member Author

Choose a reason for hiding this comment

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

The performance of coverage is already terrible, it needs to be fixed properly. One freelist more or less isn't going to make a difference.
See python/peps#2070 for something like a proper fix.

Copy link
Member

@pablogsal pablogsal Nov 24, 2021

Choose a reason for hiding this comment

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

I am skeptical of this, we are talking about the freelist of the frame object, which under coverage is going to be triggered a lot. I would like data to back this assertion.

Copy link
Member Author

@markshannon markshannon Nov 24, 2021

Choose a reason for hiding this comment

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

Which assertion? That the performance of coverage is already terrible, or that we need to fix it? 🙂

Copy link
Member

Choose a reason for hiding this comment

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

This assertion:

One freelist more or less isn't going to make a difference.

Copy link
Member

Choose a reason for hiding this comment

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

I just want to land this having an idea of the impact, not to stop the PR for landing, just to be clear

# define _PyAsyncGen_MAXFREELIST 0
# define PyContext_MAXFREELIST 0
#endif
Expand Down Expand Up @@ -158,18 +157,6 @@ struct _Py_dict_state {
#endif
};

#ifndef PyFrame_MAXFREELIST
# define PyFrame_MAXFREELIST 200
#endif

struct _Py_frame_state {
#if PyFrame_MAXFREELIST > 0
PyFrameObject *free_list;
/* number of frames currently in free_list */
int numfree;
#endif
};

#ifndef _PyAsyncGen_MAXFREELIST
# define _PyAsyncGen_MAXFREELIST 80
#endif
Expand Down Expand Up @@ -332,7 +319,6 @@ struct _is {
struct _Py_tuple_state tuple;
struct _Py_list_state list;
struct _Py_dict_state dict_state;
struct _Py_frame_state frame;
struct _Py_async_gen_state async_gen;
struct _Py_context_state context;
struct _Py_exc_state exc_state;
Expand Down
6 changes: 2 additions & 4 deletions Lib/test/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ def check(self, src, lineno, offset, encoding='utf-8'):
src = src.decode(encoding, 'replace')
line = src.split('\n')[lineno-1]
self.assertIn(line, cm.exception.text)

def test_error_offset_continuation_characters(self):
check = self.check
check('"\\\n"(1 for c in I,\\\n\\', 2, 2)
Expand Down Expand Up @@ -1342,9 +1342,7 @@ def recurse(cnt):
"""
with SuppressCrashReport():
rc, out, err = script_helper.assert_python_failure("-c", code)
self.assertIn(b'Fatal Python error: _PyErr_NormalizeException: '
b'Cannot recover from MemoryErrors while '
b'normalizing exceptions.', err)
self.assertIn(b'MemoryError', err)

@cpython_only
def test_MemoryError(self):
Expand Down
7 changes: 4 additions & 3 deletions Lib/test/test_sys.py
Original file line number Diff line number Diff line change
Expand Up @@ -1320,9 +1320,10 @@ class C(object): pass
# sys.floatinfo
check(sys.float_info, vsize('') + self.P * len(sys.float_info))
# frame
import inspect
x = inspect.currentframe()
check(x, size('3Pi3c'))
def func():
return sys._getframe()
x = func()
check(x, size('3Pi3c8P2iciP'))
# function
def func(): pass
check(func, size('14Pi'))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Allocate space for the interpreter frame in the frame object, to avoid an
additional allocation when the frame object outlives the frame activation.
1 change: 0 additions & 1 deletion Modules/gcmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1038,7 +1038,6 @@ delete_garbage(PyThreadState *tstate, GCState *gcstate,
static void
clear_freelists(PyInterpreterState *interp)
{
_PyFrame_ClearFreeList(interp);
_PyTuple_ClearFreeList(interp);
_PyFloat_ClearFreeList(interp);
_PyList_ClearFreeList(interp);
Expand Down
141 changes: 26 additions & 115 deletions Objects/frameobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ static PyGetSetDef frame_getsetlist[] = {
f_back next item on free list, or NULL
*/

static void _Py_HOT_FUNCTION
static void
frame_dealloc(PyFrameObject *f)
{
if (_PyObject_GC_IS_TRACKED(f)) {
Expand All @@ -621,9 +621,10 @@ frame_dealloc(PyFrameObject *f)
PyCodeObject *co = NULL;

/* Kill all local variables including specials, if we own them */
if (f->f_own_locals_memory) {
f->f_own_locals_memory = 0;
InterpreterFrame *frame = f->f_frame;
if (f->f_owns_frame) {
f->f_owns_frame = 0;
assert(f->f_frame == (InterpreterFrame *)f->_f_frame_data);
InterpreterFrame *frame = (InterpreterFrame *)f->_f_frame_data;
/* Don't clear code object until the end */
co = frame->f_code;
frame->f_code = NULL;
Expand All @@ -633,27 +634,10 @@ frame_dealloc(PyFrameObject *f)
for (int i = 0; i < frame->stacktop; i++) {
Py_CLEAR(locals[i]);
}
PyMem_Free(frame);
}
Py_CLEAR(f->f_back);
Py_CLEAR(f->f_trace);
#if PyFrame_MAXFREELIST > 0
struct _Py_frame_state *state = get_frame_state();
#ifdef Py_DEBUG
// frame_dealloc() must not be called after _PyFrame_Fini()
assert(state->numfree != -1);
#endif
if (state->numfree < PyFrame_MAXFREELIST) {
++state->numfree;
f->f_back = state->free_list;
state->free_list = f;
}
else
#endif
{
PyObject_GC_Del(f);
}

PyObject_GC_Del(f);
Py_XDECREF(co);
Py_TRASHCAN_END;
}
Expand All @@ -663,7 +647,7 @@ frame_traverse(PyFrameObject *f, visitproc visit, void *arg)
{
Py_VISIT(f->f_back);
Py_VISIT(f->f_trace);
if (f->f_own_locals_memory == 0) {
if (f->f_owns_frame == 0) {
return 0;
}
assert(f->f_frame->frame_obj == NULL);
Expand Down Expand Up @@ -715,11 +699,9 @@ static PyObject *
frame_sizeof(PyFrameObject *f, PyObject *Py_UNUSED(ignored))
{
Py_ssize_t res;
res = sizeof(PyFrameObject);
if (f->f_own_locals_memory) {
PyCodeObject *code = f->f_frame->f_code;
res += (code->co_nlocalsplus+code->co_stacksize) * sizeof(PyObject *);
}
res = offsetof(PyFrameObject, _f_frame_data) + offsetof(InterpreterFrame, localsplus);
PyCodeObject *code = f->f_frame->f_code;
res += (code->co_nlocalsplus+code->co_stacksize) * sizeof(PyObject *);
return PyLong_FromSsize_t(res);
}

Expand Down Expand Up @@ -747,7 +729,8 @@ static PyMethodDef frame_methods[] = {
PyTypeObject PyFrame_Type = {
PyVarObject_HEAD_INIT(&PyType_Type, 0)
"frame",
sizeof(PyFrameObject),
offsetof(PyFrameObject, _f_frame_data) +
offsetof(InterpreterFrame, localsplus),
sizeof(PyObject *),
(destructor)frame_dealloc, /* tp_dealloc */
0, /* tp_vectorcall_offset */
Expand Down Expand Up @@ -781,67 +764,21 @@ PyTypeObject PyFrame_Type = {

_Py_IDENTIFIER(__builtins__);

static InterpreterFrame *
allocate_heap_frame(PyFunctionObject *func, PyObject *locals)
static void
init_frame(InterpreterFrame *frame, PyFunctionObject *func, PyObject *locals)
{
PyCodeObject *code = (PyCodeObject *)func->func_code;
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;
}
_PyFrame_InitializeSpecials(frame, func, locals, code->co_nlocalsplus);
for (Py_ssize_t i = 0; i < code->co_nlocalsplus; i++) {
frame->localsplus[i] = NULL;
}
return frame;
}

static inline PyFrameObject*
frame_alloc(InterpreterFrame *frame, int owns)
{
PyFrameObject *f;
#if PyFrame_MAXFREELIST > 0
struct _Py_frame_state *state = get_frame_state();
if (state->free_list == NULL)
#endif
{
f = PyObject_GC_New(PyFrameObject, &PyFrame_Type);
if (f == NULL) {
if (owns) {
Py_XDECREF(frame->f_code);
Py_XDECREF(frame->f_builtins);
Py_XDECREF(frame->f_globals);
Py_XDECREF(frame->f_locals);
PyMem_Free(frame);
}
return NULL;
}
}
#if PyFrame_MAXFREELIST > 0
else
{
#ifdef Py_DEBUG
// frame_alloc() must not be called after _PyFrame_Fini()
assert(state->numfree != -1);
#endif
assert(state->numfree > 0);
--state->numfree;
f = state->free_list;
state->free_list = state->free_list->f_back;
_Py_NewReference((PyObject *)f);
}
#endif
f->f_frame = frame;
f->f_own_locals_memory = owns;
return f;
}

PyFrameObject* _Py_HOT_FUNCTION
_PyFrame_New_NoTrack(InterpreterFrame *frame, int owns)
PyFrameObject*
_PyFrame_New_NoTrack(PyCodeObject *code)
{
PyFrameObject *f = frame_alloc(frame, owns);
int slots = code->co_nlocalsplus + code->co_stacksize;
PyFrameObject *f = PyObject_GC_NewVar(PyFrameObject, &PyFrame_Type, slots);
if (f == NULL) {
return NULL;
}
Expand Down Expand Up @@ -876,15 +813,16 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code,
if (func == NULL) {
return NULL;
}
InterpreterFrame *frame = allocate_heap_frame(func, locals);
Py_DECREF(func);
if (frame == NULL) {
PyFrameObject *f = _PyFrame_New_NoTrack(code);
if (f == NULL) {
Py_DECREF(func);
return NULL;
}
PyFrameObject *f = _PyFrame_New_NoTrack(frame, 1);
if (f) {
_PyObject_GC_TRACK(f);
}
init_frame((InterpreterFrame *)f->_f_frame_data, func, locals);
f->f_frame = (InterpreterFrame *)f->_f_frame_data;
f->f_owns_frame = 1;
Py_DECREF(func);
_PyObject_GC_TRACK(f);
return f;
}

Expand Down Expand Up @@ -1087,42 +1025,15 @@ PyFrame_LocalsToFast(PyFrameObject *f, int clear)
_PyFrame_LocalsToFast(f->f_frame, clear);
}

/* Clear out the free list */
void
_PyFrame_ClearFreeList(PyInterpreterState *interp)
{
#if PyFrame_MAXFREELIST > 0
struct _Py_frame_state *state = &interp->frame;
while (state->free_list != NULL) {
PyFrameObject *f = state->free_list;
state->free_list = state->free_list->f_back;
PyObject_GC_Del(f);
--state->numfree;
}
assert(state->numfree == 0);
#endif
}

void
_PyFrame_Fini(PyInterpreterState *interp)
{
_PyFrame_ClearFreeList(interp);
#if defined(Py_DEBUG) && PyFrame_MAXFREELIST > 0
struct _Py_frame_state *state = &interp->frame;
state->numfree = -1;
#endif
}

/* Print summary info about the state of the optimized allocator */
void
_PyFrame_DebugMallocStats(FILE *out)
{
#if PyFrame_MAXFREELIST > 0
struct _Py_frame_state *state = get_frame_state();
_PyDebugAllocatorStats(out,
"free PyFrameObject",
state->numfree, sizeof(PyFrameObject));
#endif
}


Expand Down
Loading