WIP bpo-44800: Rename _PyInterpreterFrame to _Py_framedata#27525
WIP bpo-44800: Rename _PyInterpreterFrame to _Py_framedata#27525ncoghlan wants to merge 34 commits into
_PyInterpreterFrame to _Py_framedata#27525Conversation
bpo-44590 significantly improved execution speed by delaying creation of full Python objects for frames until they were needed for tracing or other introspection purposes. This follow-up commit doesn't include any further functions changes, it just changes variable, attribute, and function names to make it less ambiguous as to whether different sections of code are dealing with the classic introspection frames (full Python objects) or the new lighter weight execution frames (C structs with no intrinsic instance lifecycle management).
markshannon
left a comment
There was a problem hiding this comment.
This PR seems to be 3 PRs in one:
1 Fixes to gdbinit
2 Changing the name of InterpreterFrame to _PyExecFrame
3 Adorning many fields with "x"
-
Thanks for the fix. Would you put the fixes to
gdbinitin its own PR, please.
Do you have any ideas on how to add tests for this, to avoid anyone (probably me 🙂) breaking it again? -
Naming, the first hard problem in computer science! I think "Exec" doesn't really convey any more information than "Interpreter". Also, there should be probably be no "_Py" prefix, as that weakly implies that
_PyExecFrameis aPyObject, which it is not.
How about the old fashioned term "activation record"? The term "frame" is now so overloaded. -
I left the original names mostly unchanged for a reason. If the value of the attribute, e.g.
f_lastiis unchanged, then it makes porting code easier if the name is unchanged. Which is whyf_backbecomesprevious, because the value has changed. I'm sure there are some clarifications that could be made here, but could they go in a PR after we have decided on a name?
Finally, I'd avoid the term "introspection frame". From the point of view of Python code, the frame object is the frame, not just a view of it. Maybe phrase any comments in terms of the InterpreterFrame/_PyExecFrame/ActivationRecord being a storage optimization of the frame?
I believe that @pablogsal has been looking into writing up the way that the call stack now works, so he might have some thoughts on this.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: |
Sorry, something went wrong.
|
The main naming alternative that occurred to me is "frame" and "frame data". As far as porting goes, the extra layer of indirection is likely to be the biggest hassle, and a one time porting burden for code poking around in frame internals doesn't seem like a compelling reason to tolerate ongoing ambiguity across the code base as to which kind of structure is being used. |
Sorry, something went wrong.
|
I posted the gist of the next iteration I'm going to try to bpo issue (including checking to see if the code still feels unambiguous if the variable names are changed while leaving the struct field names alone): https://bugs.python.org/issue44800#msg399381 For gdbinit, it's just an example file that has a few other problems with it at the moment, so I think it's OK to settle on a final field naming convention before fixing the affected pieces of it. |
Sorry, something went wrong.
Where is the ambiguity? There are no field names shared between |
Sorry, something went wrong.
That is the ambiguity: situations where the only quick way to tell which kind of object you had was to have all the field names on each type memorised and infer the type from that, rather than from the types using different variable or field naming conventions. (Relying on the type declarations isn't good, as many of the affected functions are so long that most diffs in code reviews won't have the declaration readily accessible) |
Sorry, something went wrong.
|
@markshannon I have made the requested changes; please review again. I've updated the PR title and description, since the proposal has changed based on your initial review comments. I quite like the way the now proposed "frame" ( For the field prefix selection on the frame data struct, of the 12 fields, 6 didn't have a prefix at all, while 6 shared the |
Sorry, something went wrong.
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
Sorry, something went wrong.
|
Debug vs non-debug build issue (debug builds are unhappy at the moment) |
Sorry, something went wrong.
|
Partially tracked down the debug build failure - somehow Still not clear how any of the name changes could have affected when the frame state gets populated, though. |
Sorry, something went wrong.
|
As far as I can tell, it's actually legitimate to have no active Python frame when calling So while I'm still not clear on why this branch is hitting that issue when the main branch doesn't, explicitly ignoring the error seems like the right thing to do. |
Sorry, something went wrong.
|
Closing this one, see #31987 for a scaled back version of the proposed change that leaves local variable and function parameter names alone. |
Sorry, something went wrong.
Superseded by #31987
bpo-44590 significantly improved execution speed by delaying creation
of full Python objects for frames until they were needed for tracing
or other introspection purposes.
This follow-up commit doesn't include any further functional changes,
it just updates variable, attribute, and function names to make it
less ambiguous as to whether different sections of code are dealing
with frames as full Python objects or working directly with the new
lighter weight runtime frame data storage (C structs with no intrinsic
instance lifecycle management).
https://bugs.python.org/issue44800