Skip to content

GH-108035: Remove the _PyCFrame struct as it is no longer needed for performance. #108036

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

Merged
merged 5 commits into from
Aug 17, 2023

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Aug 16, 2023

This basically reverts the current_frame to the thread state as it was in 3.10.
This will need a what's new and news item, once I've benchmarked it check performance is OK.

@Eclips4 Eclips4 added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Aug 16, 2023
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

This looks like a nice reduction in complexity. I probably won't wait for it though, and merge #107760 first (and possibly the second and 3rd stage as well). You can then just update the _PUSH_FRAME op code in bytecodes.c.

@gvanrossum
Copy link
Member

Since I landed gh-107760, I figured I'd merge and fix this for you. Looks like your benchmarks come out neutral, so go ahead and merge.

@markshannon
Copy link
Member Author

Performance is in noise, maybe a tiny bit faster.

@markshannon markshannon marked this pull request as ready for review August 17, 2023 08:45
@markshannon markshannon merged commit 006e44f into python:main Aug 17, 2023
@markshannon markshannon deleted the remove-cframe branch August 18, 2023 09:52
@P403n1x87
Copy link
Contributor

@markshannon with _PyCFrame gone, how can one interleave a native stack with a Python stack reliably? Pre-3.11 we could rely on a call to PyEval_EvalFrameDefault to infer that a Python frame was being executed. On 3.11, we could use the is_entry field of _PyInterpreterFrame to infer how many Python frames were being "handled" by a call to PyEval_EvalFrameDefault. Is there a way to do a similar thing in 3.12, now that both _PyCFrame and the is_entry field are gone (I think it was removed in #96319)?

@gvanrossum
Copy link
Member

@P403n1x87 In 3.12 and 3.13 there's still this in ceval.c:

    entry_frame.owner = FRAME_OWNED_BY_CSTACK;

Shouldn't that be enough to identify the PyEval_EvalFrameDefault calls? Sure, you need to do

#include "pycore_frame.h"

but that shouldn't be a problem for your kind of application (remind me what you're working on again?)

@P403n1x87
Copy link
Contributor

@gvanrossum Thanks for bringing FRAME_OWNED_BY_CSTACK up, I was not aware of that and of its semantic. I am starting to gather information about the changes since 3.11 to add 3.12 support to Austin. Looking for occurrences of FRAME_OWNED_BY_CSTACK in ceval.c, it seems that this is only set on the entry frame. So whereas before I was checking for the is_entry field to be set, now I would check for frame.owner == FRAME_OWNED_BY_CSTACK to get the same result. Is this correct?

@gvanrossum
Copy link
Member

So whereas before I was checking for the is_entry field to be set, now I would check for frame.owner == FRAME_OWNED_BY_CSTACK to get the same result. Is this correct?

Yeah, it looks like this was mentioned in the 3.12a2 NEWS file:

When calling into Python code from C code, through
:c:func:PyEval_EvalFrameEx or a related C-API function, a shim frame in
inserted into the call stack. This occurs in the
_PyEval_EvalFrameDefault() function. The extra frame should be invisible
to all Python and most C extensions, but out-of-process profilers and
debuggers need to be aware of it. These shim frames can be detected by
checking frame->owner == FRAME_OWNED_BY_CSTACK.

Extensions implementing their own interpreters using PEP 523 need to be
aware of this shim frame and the changes to the semantics of
:opcode:RETURN_VALUE, :opcode:YIELD_VALUE, and
:opcode:RETURN_GENERATOR, which now clear the frame.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants