Improve the C code for calling Python code: _PyEval_EvalCode()

[markshannon]

BPO	42990
Nosy	@gvanrossum, @brettcannon, @rhettinger, @vstinner, @encukou, @markshannon, @serhiy-storchaka, @1st1
PRs	bpo-42990: Introduce 'frame constructor' struct to simplify API for PyEval_CodeEval and friends #24298 bpo-42990: Further refactoring of PyEval_ functions. #24368 bpo-42990: Add __builtins__ attribute to functions #24559 bpo-42990: Functions inherit current builtins #24564 bpo-42990: Refactor _PyFrame_New_NoTrack() #24566

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/markshannon'
closed_at = <Date 2021-03-17.21:00:33.189>
created_at = <Date 2021-01-21.16:05:52.112>
labels = ['interpreter-core', '3.10']
title = 'Improve the C code for calling Python code: _PyEval_EvalCode()'
updated_at = <Date 2021-03-17.21:00:33.189>
user = 'https://github.com/markshannon'

bugs.python.org fields:

activity = <Date 2021-03-17.21:00:33.189>
actor = 'vstinner'
assignee = 'Mark.Shannon'
closed = True
closed_date = <Date 2021-03-17.21:00:33.189>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2021-01-21.16:05:52.112>
creator = 'Mark.Shannon'
dependencies = []
files = []
hgrepos = []
issue_num = 42990
keywords = ['patch']
message_count = 25.0
messages = ['385432', '385491', '385500', '385908', '386061', '387187', '387190', '387192', '387196', '387199', '387223', '387233', '387243', '387244', '387245', '387260', '387304', '387306', '387340', '387341', '387345', '387377', '387401', '387408', '388969']
nosy_count = 8.0
nosy_names = ['gvanrossum', 'brett.cannon', 'rhettinger', 'vstinner', 'petr.viktorin', 'Mark.Shannon', 'serhiy.storchaka', 'yselivanov']
pr_nums = ['24298', '24368', '24559', '24564', '24566']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue42990'
versions = ['Python 3.10']

[markshannon]

Currently, to make a call to Python (modules, classes, etc, not just functions) from C has to use the monster that is _PyEval_EvalCode.
As Python has adding features over the years, _PyEval_EvalCode has grown and grown. It is time for a refactor.

_PyEval_EvalCode takes 16, yes sixteen parameters!
Those 16 parameters fall into two main groups, one describing the function being called, and the other describing the arguments to the call.
Due to the jumbo size and complexity of _PyEval_EvalCode, we also have specialised forms of it, e.g. _PyFunction_Vectorcall that handle common cases and then bail to _PyEval_EvalCode in other cases.

In outline _PyEval_EvalCode performs the following actions:

1. Make a new frame.
2. Fill in that frame using the arguments and defaults provided.
3. If the code object has flags set for a generator, or coroutine, return a new generator or coroutine.
4. Otherwise call the interpreter with the newly created frame.

As _PyEval_EvalCode or its earlier equivalents have gained arguments, they have a left of list of legacy functions. It is not clear what is the "correct" function to use in C extensions. Hopefully extension code uses the PyObject_Call API, but PyEval_EvalCodeEx is part of the C-API.

To improve this situation, I propose:

A new C struct, the "frame descriptor" which contains the code object, defaults, globals, and names that describe the code to be executed. PyFunctionObject would wrap this, to simplify the common case of calling a Python function.

Split the Eval functions into vector and tuple/dict forms, both forms taking a "frame descriptor", as well as the arguments.

Mark the older forms of the Eval functions as "legacy", creating a local "frame descriptor" and transforming the arguments in vector or tuple/dict forms, in the legacy functions.

Factor out the frame creation part of the Eval functions.

The above changes will be necessary for PEP-651, but I think would be a worthwhile improvement even if PEP-651 is not accepted.

[vstinner]

PyObject *
_PyEval_EvalCode(PyThreadState *tstate,
PyObject *_co, PyObject *globals, PyObject *locals,
PyObject *const *args, Py_ssize_t argcount,
PyObject *const *kwnames, PyObject *const *kwargs,
Py_ssize_t kwcount, int kwstep,
PyObject *const *defs, Py_ssize_t defcount,
PyObject *kwdefs, PyObject *closure,
PyObject *name, PyObject *qualname)

Hum no, only 16 arguments, everything is fine! :-D

More seriously, I already considered to rework this code.

The pseudo code is:

if (...) return <new generator>;
frame = (...);
retval = _PyEval_EvalFrame(tstate, f, 0);
_PyObject_GC_TRACK(f);
return retval;

I like the idea of splitting these two parts:

  f = create_frame_or_gen(...);
  if (<is generator>) return f;
  retval = _PyEval_EvalFrame(tstate, f, 0);
  _PyObject_GC_TRACK(f);
  return retval;

I see one advantage: stack memory consumation, we don't have to hold the 16 arguments on the stack, only 3 parameters (tstate, f, 0).

Split the Eval functions into vector and tuple/dict forms, both forms taking a "frame descriptor", as well as the arguments.

Hum, it seems like you have a different idea how to refactor the code.

Would it be worth it to have more specialized create_frame_or_gen() functions for the common cases?

--

I would also be interested by the ability to not create a frame at all, but I guess that it's a way larger refactoring.

[markshannon]

Rather than:

  f = create_frame_or_gen(...);
  if (<is generator>) return f;
  retval = _PyEval_EvalFrame(tstate, f, 0);
  _PyObject_GC_TRACK(f);
  return retval;

I was thinking:

  f = create_frame(...);
  if (<is generator>) return make_gen(f);
  retval = _PyEval_EvalFrame(tstate, f, 0);
  _PyObject_GC_TRACK(f);
  return retval;

The complicated part is create_frame(...), so I want to clean that up first.

[markshannon]

New changeset d6c33fb by Mark Shannon in branch 'master':
bpo-42990: Introduce 'frame constructor' struct to simplify API for PyEval_CodeEval and friends (GH-24298)
d6c33fb

[markshannon]

New changeset 0332e56 by Mark Shannon in branch 'master':
bpo-42990: Further refactoring of PyEval_ functions. (GH-24368)
0332e56

[gvanrossum]

+1 on exposing f,builtins.

Of course, the thing I'd really want is a way to state that all references to builtins are meant to have the exact semantics of those builtins, so the compiler can translate e.g. len(x) into a new opcode that just calls PyObject_Size(). (I can dream, can't I?)

Another dream: assume that globals that refer to modules, classes or functions don't change, so they can be cached more aggressively.

I suppose enough checking of dict version tags can get us there, or at least close enough.

[vstinner]

+1 on exposing f,builtins.

This change is related to bpo-43228 "Regression in function builtins": cloudpickle is broken by this issue (new PyFunctionObject.func_builtins member).

Of course, the thing I'd really want is a way to state that all references to builtins are meant to have the exact semantics of those builtins, so the compiler can translate e.g. len(x) into a new opcode that just calls PyObject_Size(). (I can dream, can't I?)

I tried to implement such optimization in my old https://faster-cpython.readthedocs.io/fat_python.html project. I implemented guards to de-optimize the code if a builtin is overriden.

I suppose enough checking of dict version tags can get us there, or at least close enough.

The dict version comes from my FAT Python project: PEP-509. It is used to optimize method calls. See also PEP-510 and PEP-511 ;-)

[gvanrossum]

Well maybe I'll be picking up those ideas again... Thanks for documenting
so much of what you did then!

[1st1]

I tried to implement such optimization in my old https://faster-cpython.readthedocs.io/fat_python.html project. I implemented guards to de-optimize the code if a builtin is overriden.

FWIW the globals opcode cache handles all of this now. There's no point in specifically optimizing the builtins lookup since we optimize all global lookups for a code object that's hot enough.

[gvanrossum]

FWIW the globals opcode cache handles all of this now. There's no point in specifically optimizing the builtins lookup since we optimize all global lookups for a code object that's hot enough.

So you think that even a dedicated "LEN" opcode would not be any faster? (This is getting in Paul Sokolovsky territory -- IIRC he has a variant of Python that doesn't allow overriding builtins.)

[vstinner]

New changeset a3c3ffa by Victor Stinner in branch 'master':
bpo-42990: Add __builtins__ attribute to functions (GH-24559)
a3c3ffa