gh-145497: Compute static builtin type count at compile time via X-macro

[corona10]

• Issue: Adjust _Py_MAX_MANAGED_STATIC_BUILTIN_TYPES value automatically #145497

[corona10]

@encukou @vstinner
I believe this is a clearer way to manage the list of types, since the number of types is calculated automatically once the types are registered.

[corona10]

Don't need anymore

[corona10]

ditto

[corona10]

cc @ZeroIntensity Do you have interested in this change?

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 21952b5 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F151004%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[ZeroIntensity]

Thanks for the ping. What's the net benefit here? #149139 added a test that prevents us from breaking _Py_MAX_MANAGED_STATIC_BUILTIN_TYPES in the future. I'm a little concerned that we're making things worse by adding macro gymnastics.

I think a simpler approach would be to continue using multiple static-type arrays, but export their lengths as static variables and compute _Py_MAX_MANAGED_STATIC_TYPES as the sum of their lengths. Then, we just create static_types in _PyTypes_InitTypes instead of doing it statically.

[ZeroIntensity]

I'm not a huge fan of moving all static types to their own header file. I think it makes it harder to track down where static types are being defined, because it separates the code from the definition.

[corona10]

People need to read the following code and eventually understand that they need to follow this pattern:

static PyTypeObject* static_types[] = {
    _Py_FOREACH_STATIC_PREINIT_TYPE(_ADD_TYPE)
};

We could add a comment if necessary, but TBH, the current declaration is not very readable either because it is already quite long.

[zooba]

I like it, it makes it more annoying to add a new static type, which will hopefully lead someone to actually think about whether it should be static or not (and not just make it static because it seems easiest).

Could we have a little comment by the lists setting out the criteria for making something static vs. per-interpreter (and where to look to define something as per-interpreter)?

[corona10]

@ZeroIntensity

Thanks for the ping. What's the net benefit here? #149139 added a test that prevents us from breaking _Py_MAX_MANAGED_STATIC_BUILTIN_TYPES in the future. I'm a little concerned that we're making things worse by adding macro gymnastics.

The main motivation is to avoid having to manually update the number of types.
Also, in #145497 (comment), Petr mentioned that the current check is only a workaround.
So I think the benefit is that the count is computed at compile time from the list of registered static types, instead of being another manually maintained value.

I think a simpler approach would be to continue using multiple static-type arrays, but export their lengths as static variables and compute _Py_MAX_MANAGED_STATIC_TYPES as the sum of their lengths. Then, we just create static_types in _PyTypes_InitTypes instead of doing it statically.

I considered that approach too, but it would require switching some currently inline arrays (e.g. interp->types.builtins.initialized[]) to dynamic allocation, and I wanted to avoid the complications that come with that.
I just prefer to focus on not to touch derefernce code outside.
(My recall could be wrong please tackle this if you think other way)

[vstinner]

I dislike moving static_types and static_exceptions arrays to a new Include/internal/pycore_static_builtin_types.h header file. The long _Py_FOREACH macros seem to be harder to maintain, you removed comments for example.

I would prefer to leave static_types and static_exceptions where they are currently defined, but use the _Py_FOREACH macro to count "extra types".

So I wrote PR gh-151152 to do that. The change is smaller and it should be easier to maintain in the long term.

[corona10]

@zooba

I like it, it makes it more annoying to add a new static type, which will hopefully lead someone to actually think about whether it should be static or not (and not just make it static because it seems easiest).

Well, I am very sad if this make people angry, I could consider approach if there is a good way.

[corona10]

@vstinner I think that the PR you suggested can not solve original problem that we are meeting, we don't want to manage each number of types by hands when interpreter should care about it.

[ZeroIntensity]

I like it, it makes it more annoying to add a new static type, which will hopefully lead someone to actually think about whether it should be static or not (and not just make it static because it seems easiest).

If you try to add a new static type right now, you'll already get a C analyzer failure, so you have to think about it a lot anyway,

I considered that approach too, but it would require switching some currently inline arrays (e.g. interp->types.builtins.initialized[]) to dynamic allocation, and I wanted to avoid the complications that come with that.

Looking at the code, I don't think we'd need to do that. Wouldn't something like this work?

PyStatus
_PyTypes_InitTypes(PyInterpreterState *interp)
{
    PyTypeObject static_types[computed_sum] = ...;
    for (size_t i=0; i < Py_ARRAY_LENGTH(static_types); i++) {
        PyTypeObject *type = static_types[i];
        /* ... */
    }
}

[corona10]

Looking at the code, I don't think we'd need to do that. Wouldn't something like this work?

Hmm let me check your approach.

[vstinner]

@corona10:

@vstinner I think that the PR you suggested can not solve original problem that we are meeting, we don't want to manage each number of types by hands when interpreter should care about it.

Replacing static_exceptions[] with static_exceptions[_Py_NUM_STATIC_EXCEPTIONS] emits a compiler warning if an exception is added without updating _Py_NUM_STATIC_EXCEPTIONS. It's possible to miss a compiler warning, but it's better than the current status where no warning is emitted. Same for static_types[_Py_NUM_MANAGED_PREINITIALIZED_TYPES].

[vstinner]

@ZeroIntensity @zooba: Do you have a preference between this PR and my PR gh-151152?

IMO this PR is more correct, but it makes the code harder to maintain.

[ZeroIntensity]

I don't have much of a preference -- I'm not a fan of the #define _PY_COUNT_STATIC_TYPE_(name) + 1 hack in either case.

[corona10]

I don't have much of a preference -- I'm not a fan of the #define PY_COUNT_STATIC_TYPE(name) + 1 hack in either case.

Well the hack is the essential part of this PR unless we can use C++ in our codebase ;)

[corona10]

@vstinner @ZeroIntensity

This is my proposed middle ground: this change does not break compatibility or introduce any real risk. If we later decide it is not the right direction, we can revert it or take a different approach. Alternatively, we may be able to improve it further in a way that makes it easier to maintain.

What do you think about this compromise?

[zooba]

I can't find anywhere we even use these constants, so maybe we should just stick to the Py_ARRAY_LENGTH where it is and stop worrying about putting the total count anywhere?

If we don't need to publish the count, then I prefer keeping the list in one central place that isn't reusable, which would probably be object.c. Anyone who needs it should call a function in object.c to get access, but otherwise, let's keep it all private.

[vstinner]

@zooba:

If we don't need to publish the count, then I prefer keeping the list in one central place that isn't reusable, which would probably be object.c. Anyone who needs it should call a function in object.c to get access, but otherwise, let's keep it all private.

The problem is that we need to know _Py_MAX_MANAGED_STATIC_TYPES value at build time, since it's used to define types array in Include/internal/pycore_interp_structs.h. We cannot call functions at runtime (to get the size).

#define _Py_MAX_MANAGED_STATIC_TYPES ...

struct _types_runtime_state {
    ...

    struct {
        struct {
            PyTypeObject *type;
            int64_t interp_count;
        } types[_Py_MAX_MANAGED_STATIC_TYPES];
    } managed_static;
};

[zooba]

Ah, I only searched for _Py_NUM_[A-Z_]+_TYPES, not _Py_MAX..., that's why I didn't find it.

But also, it's part of the runtime state, which doesn't have to be valid until we initialize. So why can't we allocate it at runtime?