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.

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

Jump to bottom

bpo-45256: Don't track the exact depth of each InterpreterFrame #30372

Merged
merged 7 commits into from
Jan 5, 2022
Merged

bpo-45256: Don't track the exact depth of each InterpreterFrame #30372

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2713a36
Stop tracking the exact depth of each frame
brandtbucher Jan 3, 2022
439a0c2
Clean up comment
brandtbucher Jan 3, 2022
6037cba
Fix test for sizeof(frame)
brandtbucher Jan 3, 2022
323da04
Fix test for sizeof(generator)
brandtbucher Jan 3, 2022
1ab3a8f
Catch up with main
brandtbucher Jan 4, 2022
98c58f3
Apply suggestions from code review
brandtbucher Jan 4, 2022
369983b
Don't bother unsetting frame->is_entry
brandtbucher Jan 4, 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
6 changes: 4 additions & 2 deletions Include/internal/pycore_frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
extern "C" {
#endif

#include <stdbool.h>


/* runtime lifecycle */

Expand Down Expand Up @@ -44,7 +46,7 @@ typedef struct _interpreter_frame {
int f_lasti; /* Last instruction if called */
int stacktop; /* Offset of TOS from localsplus */
PyFrameState f_state; /* What state the frame is in */
int depth; /* Depth of the frame in a ceval loop */
bool own_cframe; // Whether this is the "root" frame for the current CFrame
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this name.

I don't think the frame really owns the C frame. Also, be wary of using CFrame, as CFrame is a struct name.
If I were to suggest a name, it would be is_entry as this is the entry InterpreterFrame for _PyEval_EvalFrameDefault.
If you think ownership is a better mental model, then owns_cframe is fine.

Copy link
Member

Choose a reason for hiding this comment

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

@markshannon Can it be named backed_by_cframe or cframe_backed instead?

Copy link
Member

Choose a reason for hiding this comment

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

This has nothing to do with how the data is stored.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Mark, I think it should be called entry_frame or something similar. Reasoning about ownership is a bit more obscure in my opinion

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 was hoping somebody would suggest a better name! is_entry it is...

PyObject *localsplus[1];
} InterpreterFrame;

Expand Down Expand Up @@ -101,7 +103,7 @@ _PyFrame_InitializeSpecials(
frame->generator = NULL;
frame->f_lasti = -1;
frame->f_state = FRAME_CREATED;
frame->depth = 0;
frame->own_cframe = true;
Copy link
Member

Choose a reason for hiding this comment

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

This should default to false. Then it only need be set at the start of _PyEval_EvalFrameDefault, not for every internal call.

}

/* Gets the pointer to the locals array
Expand Down
16 changes: 8 additions & 8 deletions Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1683,7 +1683,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
cframe.previous = prev_cframe;
tstate->cframe = &cframe;

assert(frame->depth == 0);
assert(frame->own_cframe);
/* Push frame */
frame->previous = prev_cframe->current_frame;
cframe.current_frame = frame;
Expand Down Expand Up @@ -2310,7 +2310,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
_PyFrame_SetStackPointer(frame, stack_pointer);
new_frame->previous = frame;
frame = cframe.current_frame = new_frame;
new_frame->depth = frame->depth + 1;
frame->own_cframe = false;
goto start_frame;
}

Expand Down Expand Up @@ -2475,7 +2475,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
TRACE_FUNCTION_EXIT();
DTRACE_FUNCTION_EXIT();
_Py_LeaveRecursiveCall(tstate);
if (frame->depth) {
if (!frame->own_cframe) {
frame = cframe.current_frame = pop_frame(tstate, frame);
_PyFrame_StackPush(frame, retval);
goto resume_frame;
Expand Down Expand Up @@ -2625,7 +2625,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
}

TARGET(SEND) {
assert(frame->depth == 0);
assert(frame->own_cframe);
assert(STACK_LEVEL() >= 2);
PyObject *v = POP();
PyObject *receiver = TOP();
Expand Down Expand Up @@ -2684,7 +2684,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
}

TARGET(YIELD_VALUE) {
assert(frame->depth == 0);
assert(frame->own_cframe);
PyObject *retval = POP();

if (frame->f_code->co_flags & CO_ASYNC_GENERATOR) {
Expand Down Expand Up @@ -4645,7 +4645,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
_PyFrame_SetStackPointer(frame, stack_pointer);
new_frame->previous = frame;
cframe.current_frame = frame = new_frame;
new_frame->depth = frame->depth + 1;
frame->own_cframe = false;
goto start_frame;
}
}
Expand Down Expand Up @@ -4739,7 +4739,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
_PyFrame_SetStackPointer(frame, stack_pointer);
new_frame->previous = frame;
frame = cframe.current_frame = new_frame;
new_frame->depth = frame->depth + 1;
frame->own_cframe = false;
goto start_frame;
}

Expand Down Expand Up @@ -5415,7 +5415,7 @@ MISS_WITH_OPARG_COUNTER(STORE_SUBSCR)
exit_unwind:
assert(_PyErr_Occurred(tstate));
_Py_LeaveRecursiveCall(tstate);
if (frame->depth == 0) {
if (frame->own_cframe) {
/* Restore previous cframe and exit */
tstate->cframe = cframe.previous;
tstate->cframe->use_tracing = cframe.use_tracing;
Expand Down
10 changes: 5 additions & 5 deletions Tools/gdb/libpython.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,8 +1044,8 @@ def _f_nlocalsplus(self):
def _f_lasti(self):
return self._f_special("f_lasti", int_from_int)

def depth(self):
return self._f_special("depth", int_from_int)
def own_cframe(self):
return self._f_special("own_cframe", bool)

def previous(self):
return self._f_special("previous", PyFramePtr)
Expand Down Expand Up @@ -1860,7 +1860,7 @@ def print_summary(self):
line = interp_frame.current_line()
if line is not None:
sys.stdout.write(' %s\n' % line.strip())
if interp_frame.depth() == 0:
if interp_frame.own_cframe():
break
else:
sys.stdout.write('#%i (unable to read python frame information)\n' % self.get_index())
Expand All @@ -1883,7 +1883,7 @@ def print_traceback(self):
line = interp_frame.current_line()
if line is not None:
sys.stdout.write(' %s\n' % line.strip())
if interp_frame.depth() == 0:
if interp_frame.own_cframe():
break
else:
sys.stdout.write(' (unable to read python frame information)\n')
Expand Down Expand Up @@ -2147,7 +2147,7 @@ def invoke(self, args, from_tty):
% (pyop_name.proxyval(set()),
pyop_value.get_truncated_repr(MAX_OUTPUT_LEN)))

if pyop_frame.depth() == 0:
if pyop_frame.own_cframe():
break

pyop_frame = pyop_frame.previous()
Expand Down