bpo-43693: Add _PyCode_New() and do some related cleanup. #26258

ericsnowcurrently · 2021-05-19T23:51:47Z

The key change here is the addition of _PyCode_New(), which is an internal-only API to replace use of PyCode_NewWithPosOnlyArgs(). The problem with that older API is the it has so many parameters. This becomes especially painful when modifying its signature. The new API uses a single struct to hold the values needed to create a new PyCodeObject instance.

Other changes:

added co_nfastlocals, co_ncellvars, and co_nfreevars; this matters because I plan on replacing co_varnames, etc. with a single consolidated tuple
moved some of the code tied to the internal resolution of co_varnames, etc. from ceval.c to codeobject.c (as functions); this reduced coupling makes it easier to change how PyCodeObject works internally
re-arranged the PyCodeObject fields so they are grouped a bit more logically

There is still code in ceval.c (_PyEval_MakeFrameVector() specifically, and pyframeobject.c that is coupled to the internal structure of PyCodeObject, but this PR is big enough already. 🙂

https://bugs.python.org/issue43693

markshannon · 2021-05-20T10:33:49Z

I agree that we should add a non-API function for creating code objects and _PyCodeNew seems like a sensible name for it.
But surely that should be a 10 line diff, not a 1000 line diff?

Passing a struct as a single parameter to a function means that you have to ensure that all future versions can handling trailing zeroes.
E.g.

PyObject *make_thing(struct thing *t);

Version 1:

struct thing {
    some data;
};

Version 2:

struct thing {
   some data;
   int super_important_must_never_be_zero;

Any client code written for version 1 will compile for version 2 and then crash mysteriously when run.

ericsnowcurrently · 2021-05-20T15:22:34Z

I see your point but consider it less of an issue for an internal-only API like this. For the most part this PR is about addressing the pain points I ran into while making changes around PyCodeObject:

changes to the public "constructor" API to accommodate changes to internal details are frustrating for everyone (hence the new internal function)
much of the public API for PyCodeObject probably shouldn't be public in the first place
the signature of 2 of the "constructor" functions are particularly large, so adding, removing, or re-arranging parameters is annoying (hence the args struct, which we also do for frames)
for folks not already familiar with PyCodeObject, the nature of the various fields isn't obvious at first (hence grouping the fields)
more than a couple modules are fairly tightly coupled to the internal details of PyCodeObject, which may make sense for ceval.c as the confluence of functions, code, and frames, but that coupling (even in ceval,c) means changes to PyCodeObject internals result in touches to a variety of other modules (and the number of modules touched for a change correlates to bug count)

I'm okay with dialing back the changes if you think they aren't worth it, but the definitely are based in my experience thus far with PyCodeObject.

markshannon · 2021-05-20T17:06:58Z

I think we all agree that the API for making PyCodeObjects shouldn't have been public in the first place. But it is 😞
I'm in favour of removing it, but that would need a proper discussion of python-dev.

The accessor macros, _PyCode_GET_INSTRUCTION etc, seem useful, but could you make them inline functions.

Ultimately we want something like https://github.com/markshannon/peps/blob/pep-mappable-pyc-file/pep-066x.rst#runtime-objects
It is going to take a while to get there, but let's try and minimize the changes along the way.

markshannon · 2021-05-22T10:58:52Z

Please remove construction of a CodeObject using a structure. As I explained earlier it isn't safe.
Nor does it do anything for clarity, IMO, it just adds bulk.

bl-ue

Hi, I found a few things worth taking a look at :)

Include/cpython/code.h

Include/internal/pycore_code.h

Objects/frameobject.c

gvanrossum · 2021-05-22T15:25:52Z

Please remove construction of a CodeObject using a structure. As I explained earlier it isn't safe.

That only applies for public APIs. This is a purely internal API. The compiler will catch the case you worry about in that explanation.

Nor does it do anything for clarity, IMO, it just adds bulk.

It allows us something not entirely unlike keyword arguments in C, which I find a big gain to readability.

markshannon · 2021-05-23T10:07:20Z

No, the compiler won't catch it. It is legal to leave trailing fields off a struct assignment.
It only works if NULL or 0 is a sensible value for all the fields. For this struct, that is not the case. Even if it were, we would have laid a trap for future developers adding new fields.

ericsnowcurrently · 2021-05-24T15:26:26Z

@markshannon, calling check_code() will catch the problem you described. That said, I don't see this as a problem for this internal API. As to readability. It's what Guido said. To elaborate,: the order doesn't matter and you can leave "args" off if you want to use the default.

Regardless, there isn't a lot of value to a long discussion about this. If you feel strongly about this then I'll change it to a function with many parameters.

Include/internal/pycore_code.h

markshannon

This PR is too big for me to be confident that I'm not missing something important.
Why does it need to be so large?

There are a lot of changes to frameobject.c, typeobject.c and ceval.c that seem unrelated.
Several of which look like they will degrade performance.

markshannon · 2021-05-25T14:20:19Z

Include/cpython/code.h

    PyObject *co_consts;        /* list (constants used) */
    PyObject *co_names;         /* list of strings (names used) */
+


co_code, co_consts and co_names are the hottest fields in the code object. Could make sure that they are near the start of the object. Moving the metadata section after this section should do.

markshannon · 2021-05-25T14:21:47Z

Include/internal/pycore_code.h

@@ -24,9 +27,147 @@ struct _PyOpcache {
    char optimized;
 };

+
+// We would use an enum if C let us specify the storage type.


You can still use an enum to define the values though, even if you store them in a char.

markshannon · 2021-05-25T14:25:24Z

Include/internal/pycore_code.h

+}
+
+static inline bool
+_PyCode_CodeIsValid(PyCodeObject *co)


This doesn't check for validity, at all. A full validity check would be large and slow. Maybe rename to reflect what it does?

markshannon · 2021-05-25T14:27:39Z

Include/internal/pycore_code.h

+}
+
+static inline _Py_CODEUNIT *
+_PyCode_GetInstructions(PyCodeObject *co)


This is going to be misleading with quickening, so could you drop it for now.
Plus these accessor functions have an annoying tendency to end up in the public API, historically.

markshannon · 2021-05-25T14:35:23Z

Objects/codeobject.c

-                          PyObject *filename, PyObject *name, int firstlineno,
-                          PyObject *linetable, PyObject *exceptiontable)
+static int
+check_code(struct _PyCodeConstructor *con, enum check_mode mode)


Using boolean parameters (and check_mode is a boolean) that change behavior is generally regarded as bad API design.
I think it would be better if the legacy form just cleared the exception then called PyErr_BadInternalCall()

markshannon · 2021-05-25T15:11:04Z

Objects/frameobject.c

@@ -649,6 +649,9 @@ frame_dealloc(PyFrameObject *f)
 static inline Py_ssize_t
 frame_nslots(PyFrameObject *frame)
 {
+    assert(frame->f_valuestack - frame->f_localsptr ==
+           // fastlocals + builtins + globals + locals
+           frame->f_code->co_nfastlocals + 3);


OK, I see it is just misnamed, and the comment is misleading. Cell and free variables are also included.

markshannon · 2021-05-25T15:11:53Z

Objects/frameobject.c

@@ -917,146 +920,28 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code,
    return f;
 }

-/* Convert between "fast" version of locals and dictionary version.


Why all these changes? This seems unrelated to adding _PyCode_New().

markshannon · 2021-05-25T15:13:43Z

Objects/typeobject.c

@@ -8830,71 +8831,50 @@ static int
 super_init_without_args(PyFrameObject *f, PyCodeObject *co,
                        PyTypeObject **type_p, PyObject **obj_p)
 {
-    if (co->co_argcount == 0) {
+    if (!_PyCode_HasFastlocals(co, CO_FAST_POSONLY | CO_FAST_POSORKW)) {


Why? This is performance critical code, and this looks a lot slower.

markshannon · 2021-05-25T15:26:27Z

Objects/codeobject.c

+}
+
+int
+_PyCode_OffsetFromIndex(PyCodeObject *co, int index, _PyFastLocalKind kind)


I don't like the use of this function everywhere.
If inlined, it should be no slower than the code it is replacing, but what is the point?
index + co->co_nlocals is clearer to me than _PyCode_OffsetFromIndex(co, index, CO_FAST_CELL).
index is far, far clearer than _PyCode_OffsetFromIndex(co, index, CO_FAST_LOCAL)`

markshannon · 2021-05-25T15:27:51Z

Python/ceval.c

    if (co->co_flags & CO_VARKEYWORDS) {
        kwdict = PyDict_New();
        if (kwdict == NULL)
            goto fail;
-        i = total_args;
+        int offset = _PyCode_OffsetFromIndex(co, total_args, CO_FAST_LOCAL);


This is performance critical code and this is going to be slower unless _PyCode_OffsetFromIndex is inlined.

bedevere-bot · 2021-05-25T15:37:55Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

ericsnowcurrently · 2021-05-25T17:34:13Z

If this PR is too big to review well enough then I'll split it up.

ericsnowcurrently · 2021-05-26T01:04:00Z

I'm closing this in favor of #26364 and #26375. There are more changes than those cover, but the rest can be dealt with (or abandoned) later.

ericsnowcurrently requested a review from markshannon May 19, 2021 23:51

ericsnowcurrently requested a review from tiran as a code owner May 19, 2021 23:51

the-knights-who-say-ni added the CLA signed label May 19, 2021

bedevere-bot added the awaiting core review label May 19, 2021

ericsnowcurrently added the skip news label May 19, 2021

ericsnowcurrently force-pushed the code-object-cleanup branch from 23ec578 to 2a3de5d Compare May 20, 2021 17:26

ericsnowcurrently added 21 commits May 21, 2021 09:04

Re-organize the _PyCodeObject fields.

dce5124

Spell out the PyCode_New*() signatures.

e416918

Add a TODO to code.h.

6740572

Fix a memory leak.

4381869

Add _PyCode_New().

19cfb8b

Factor out _PyCode_GetFastlocalOffsetId().

8f10f5d

Factor out _PyCode_HasFastlocals().

0c42221

Factor out _PyCode_CellForLocal().

3315b28

Add macros to hide away some of the PyCodeObject fields.

2ac48ba

Move the fast locals section down.

02e15a4

Store more counts on PyCodeObject.

ad70f99

Update a comment.

e38643f

Factor out macros related to co_code.

09fbe71

Fix some smelly names.

9b4ca77

Fix whitespace.

f00133f

Fix co_nlocals in code.replace().

acef742

Stop using statics in PyCode_NewEmpty().

559e408

Actually fix whitespace.

a923b5b

Fix a typo.

afcc944

Replace macros with inline functions.

d304aba

Unify all the PyCodeObject validation code.

23e5485

ericsnowcurrently added 2 commits May 21, 2021 20:18

Regen a couple more files.

fd50f62

Fix test_ctypes.

9736ff5

bl-ue reviewed May 22, 2021

View reviewed changes

Include/cpython/code.h Show resolved Hide resolved

Include/cpython/code.h Outdated Show resolved Hide resolved

Include/internal/pycore_code.h Outdated Show resolved Hide resolved

Objects/frameobject.c Outdated Show resolved Hide resolved

ericsnowcurrently added 4 commits May 24, 2021 09:35

Factor out CO_FAST_ARG.

c95314c

Fix a typo.

b419060

Add an assert for the return value of frame_nslots().

6195184

Add a comment about using an arguments struct for _PyCode_New().

3c4ec94

bl-ue reviewed May 24, 2021

View reviewed changes

Include/internal/pycore_code.h Outdated Show resolved Hide resolved

ericsnowcurrently added 9 commits May 24, 2021 12:24

Drop some outdated TODO comments.

ffc4b54

Fix a typo.

292b646

Add _PyCode_FastInfoFromOffset().

cd9e34a

Add _PyCode_GetFastFreevar().

cd4f587

Use _PyCode_FastInfoFromOffset() elsewhere in ceval.c.

a4e6dd3

Add _PyCode_OffsetFromIndex().

f93b6df

_PyCode_FastInfoFromOffset() -> _PyCode_FastInfoFromOparg().

f779627

Add _PyCode_FastInfoFromOffset() and use it in frameobject.c.

292e457

Fix warnings.

f6bafb4

markshannon requested changes May 25, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels May 25, 2021

ericsnowcurrently closed this May 26, 2021

bl-ue mentioned this pull request May 31, 2021

Add 91 typo variants accumulated over the past two weeks codespell-project/codespell#1927

Closed

markshannon mentioned this pull request May 2, 2022

Logically merge cell and locals array. They are already contiguous in memory #87859

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-43693: Add _PyCode_New() and do some related cleanup. #26258

bpo-43693: Add _PyCode_New() and do some related cleanup. #26258

ericsnowcurrently commented May 19, 2021 •

edited by bedevere-bot

Loading

markshannon commented May 20, 2021

ericsnowcurrently commented May 20, 2021

markshannon commented May 20, 2021

markshannon commented May 22, 2021

bl-ue left a comment

gvanrossum commented May 22, 2021

markshannon commented May 23, 2021

ericsnowcurrently commented May 24, 2021

markshannon left a comment

markshannon May 25, 2021

markshannon May 25, 2021

markshannon May 25, 2021

markshannon May 25, 2021

markshannon May 25, 2021

markshannon May 25, 2021

markshannon May 25, 2021

markshannon May 25, 2021

markshannon May 25, 2021

markshannon May 25, 2021

bedevere-bot commented May 25, 2021

ericsnowcurrently commented May 25, 2021

ericsnowcurrently commented May 26, 2021

		PyObject co_consts; / list (constants used) */
		PyObject co_names; / list of strings (names used) */

bpo-43693: Add _PyCode_New() and do some related cleanup. #26258

bpo-43693: Add _PyCode_New() and do some related cleanup. #26258

Conversation

ericsnowcurrently commented May 19, 2021 • edited by bedevere-bot Loading

markshannon commented May 20, 2021

ericsnowcurrently commented May 20, 2021

markshannon commented May 20, 2021

markshannon commented May 22, 2021

bl-ue left a comment

Choose a reason for hiding this comment

gvanrossum commented May 22, 2021

markshannon commented May 23, 2021

ericsnowcurrently commented May 24, 2021

markshannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented May 25, 2021

ericsnowcurrently commented May 25, 2021

ericsnowcurrently commented May 26, 2021

ericsnowcurrently commented May 19, 2021 •

edited by bedevere-bot

Loading