Skip to content

bpo-45256: Avoid C calls for more Python to Python calls. #28937

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

Merged
markshannon merged 6 commits into from
Oct 18, 2021
+127 −179
Merged

bpo-45256: Avoid C calls for more Python to Python calls. #28937

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78b869e
Avoid making C calls for most calls to Python functions.
markshannon Oct 11, 2021
73041b5
Refactor a bit to help parsing of instructions.
markshannon Oct 13, 2021
5cc5442
Update gdb test to account for fewer C calls.
markshannon Oct 13, 2021
7c0f498
Change initialize_locals(steal=true) and _PyTuple_FromArraySteal to c…
markshannon Oct 14, 2021
03e7ad9
Fix compiler warnings on Windows.
markshannon Oct 14, 2021
82f5093
Rename labels and variable for clarity.
markshannon Oct 14, 2021
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
Prev Previous commit
Next Next commit
Refactor a bit to help parsing of instructions.
  • Loading branch information
@markshannon
markshannon committed Oct 13, 2021
commit 73041b5e7594f3ead689c766124f2eee08cef5e1
29 changes: 13 additions & 16 deletions Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1702,6 +1702,11 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
switch (opcode) {
#endif

/* Variables used for making calls */
PyObject *kwnames;
Copy link
Copy Markdown
Member

@pablogsal pablogsal Oct 14, 2021

Choose a reason for hiding this comment

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

Could we put this into a struct instead of having more locals? It makes it a bit more clean to read and contextualize and it will have the same performance as long as the struct is stack allocated

Copy link
Copy Markdown
Member Author

@markshannon markshannon Oct 14, 2021

Choose a reason for hiding this comment

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

Do we want to rely on all compilers being able to do perfect escape analysis here?
I'd rather leave these as local variables so the compiler can easily allocate them to registers?

Copy link
Copy Markdown
Member

@pablogsal pablogsal Oct 14, 2021

Choose a reason for hiding this comment

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

There is no way that given the size of this function this is going to end in registers. Indeed: I checked with GCC, clang, ICC and xlc in all different optimization level and not a single one places these locals on registers.

Up to you anyway, I don't feel super strongly about it but I think it helps with clarity and organization.

Copy link
Copy Markdown
Member Author

@markshannon markshannon Oct 15, 2021

Choose a reason for hiding this comment

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

How did you determine which of these were in registers? Once in SSA form, there are 15 (I think) variables here.

Copy link
Copy Markdown
Member

@pablogsal pablogsal Oct 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

gdb/dbx + breakpoint for this function + disas + info registers. I checked if any of the values in these locals are stored or partially stored in registers.

I only checked x86-64 thought.

int nargs;
int stackadj;
Comment thread
markshannon marked this conversation as resolved.
Outdated

/* BEWARE!
It is essential that any operation that fails must goto error
and that all operation that succeed call DISPATCH() ! */
Expand Down Expand Up @@ -4538,11 +4543,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
DISPATCH();
}

/* Declare variables used for making calls */
PyObject *kwnames;
int nargs;
int stackadj;

TARGET(CALL_METHOD) {
/* Designed to work in tamdem with LOAD_METHOD. */
/* `meth` is NULL when LOAD_METHOD thinks that it's not
Expand Down Expand Up @@ -4589,25 +4589,22 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
goto call_function;
}

TARGET(CALL_FUNCTION) {
PREDICTED(CALL_FUNCTION);
nargs = oparg;
kwnames = NULL;
stackadj = 1;
goto call_function;
}

TARGET(CALL_FUNCTION_KW) {
kwnames = POP();
nargs = oparg - PyTuple_GET_SIZE(kwnames);
stackadj = 1;
goto call_function;
}

call_function:
{
TARGET(CALL_FUNCTION) {
PREDICTED(CALL_FUNCTION);
PyObject *function;
nargs = oparg;
kwnames = NULL;
stackadj = 1;
call_function:
// Check if the call can be inlined or not
PyObject *function = PEEK(oparg + 1);
function = PEEK(oparg + 1);
if (Py_TYPE(function) == &PyFunction_Type && tstate->interp->eval_frame == NULL) {
int code_flags = ((PyCodeObject*)PyFunction_GET_CODE(function))->co_flags;
int is_generator = code_flags & (CO_GENERATOR | CO_COROUTINE | CO_ASYNC_GENERATOR);
Expand Down