Skip to content

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