bpo-45256: Don't track the exact depth of each InterpreterFrame
#30372
Conversation
Include/internal/pycore_frame.h
Outdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@markshannon Can it be named backed_by_cframe or cframe_backed instead?
There was a problem hiding this comment.
This has nothing to do with how the data is stored.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I was hoping somebody would suggest a better name! is_entry it is...
Include/internal/pycore_frame.h
Outdated
| frame->f_lasti = -1; | ||
| frame->f_state = FRAME_CREATED; | ||
| frame->depth = 0; | ||
| frame->own_cframe = true; |
There was a problem hiding this comment.
This should default to false. Then it only need be set at the start of _PyEval_EvalFrameDefault, not for every internal call.
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
A couple of minor issues, but sound in general.
|
When you're done making the requested changes, leave the comment: |
6 participants
We only really care whether or not an exiting frame is the "root" of the current
CFrame, which a boolean value can track more efficiently.(In fact, the current code is buggy, and doesn't even maintain
frame->depthcorrectly... it's only ever set to0or1anyways!)https://bugs.python.org/issue45256