Statically initialize PyArg_Parser variables in clinic.py.

[ericsnowcurrently]

https://bugs.python.org/issue46772

Changes will be in Tools/clinic/clinic.py.

[ericsnowcurrently]

blocked by #285 and #287

[kumaraditya303]

I have a rough plan of how this can be implemented, and to remove code duplication between deepfreeze and clinic, it can be factored into a separate module.

Clinic can generate the static tuple object and since the keywords are identifiers, it can use _Py_ID macro to declare the keywords in the ob_item array. Since there won't be any changes to the generated tuple object, it can be even be made static const if possible to move to the read only section of the executable which will be faster, parser_init could be made to check the generated tuple in debug mode and in release builds it will be simply removed which should make parsing overhead less as it would avoid a branch, and finally the parsers would not have to be deallocated simplifying runtime shutdown and this would be subinterpreter friendly.

[kumaraditya303]

@ericsnowcurrently Have you started working on this or you have a draft branch with using internal APIs?

[ericsnowcurrently]

Yeah, I've been working on this. However, feel free to take over! 🙂 It sounds like you have a similar understanding to mine on what needs to be done. I have plenty of context at this point and would be glad to talk through any of the details with you. I can also show you my branch if you like.

[kumaraditya303]

I can also show you my branch if you like.

Yes, I would like to take a look. thanks

[ericsnowcurrently]

to remove code duplication between deepfreeze and clinic, it can be factored into a separate module.

FWIW, I agree that there is significant overlap between clinic and deepfreeze (as well as other tools like generate_frozen_modules.py). Factoring our common parts may be a good idea. Just remember that the tools aren't as carefully tested as the CPython runtime and that it's easy to introduce bugs when refactoring (especially large-scale refactoring). That doesn't mean it's a bad idea. We just need to be consider the possible downsides seriously.

I'll also note that the approach clinic takes a substantially different approach for code generation compared to deepfreeze. Deepfreeze generates the output as it goes, with some structural helpers to simplify the process (@gvanrossum's design). In contrast, clinic does the following (more or less):

1. parse input
2. generate a template
3. apply the parsed data to the template

The clinic approach makes it much harder to generate concise code for things like statically initialized _PyArg_Parser structs. The deepfreeze approach is much better suited, so from that standpoint I like the idea of refactoring clinic to share the deepfreeze approach. I'd even considered it briefly. However, it looked like way more work than I have time to spare. 🙂

If you want to give it a shot then I say go for it. Just don't be afraid to fight through the challenge. And don't be afraid to abandon the change if it starts looking like it's too much to handle or if the level of churn to too high relative to the benefit. (You can sometimes get away with a little more churn in tools than in runtime code, but don't push it.)

[ericsnowcurrently]

I can also show you my branch if you like.

Yes, I would like to take a look. thanks

python/cpython@main...ericsnowcurrently:global-objects-pyargs-parser-keywords-init

I've temporarily dropped all generated code from the branch to make it easier to see the actual changes.

[ericsnowcurrently]

Also, let's move any further discussion on this to https://bugs.python.org/issue46772.

[kumaraditya303]

The changes so far looks good to me and to reduce the size of the changes perhaps it can be committed as is and improve in later small PRs.
Some out of scope things and improvements (hence commenting here) which are worth doing separately:

• Make _PyArg_Parser constant (immutable). Instead of doing work at runtime it can be done at AC generation time like scan_keywords which checks for empty kwname and counts total number of positional-only parameters as it can be done in AC generation time.
• Remove parser_init and do parse_format at AC generation time.
• Finally remove list of static argparsers static_arg_parsers
• This would remove most of the overhead from parsing of arguments.

Once python/cpython@main...ericsnowcurrently:global-objects-pyargs-parser-keywords-init lands I'll work on some of these improvements.

[ericsnowcurrently]

Yeah, all those things make sense doing once all the fields are statically initialized.

[kumaraditya303]

@ericsnowcurrently 3.12 dev has now started so this would be great to get in so that we have more time to improve and fix bugs. I tried your branch and it segfaults when running the test suite in test_builtin, Can you rebase this to the latest main branch?

[ericsnowcurrently]

I'll do that as soon as I possible, though it probably won't be right away. In the meantime, feel free to copy my branch and merge main if you want to get things rolling faster.

[kumaraditya303]

I have a WIP branch kumaraditya303/cpython#4. There are two things to investigate further:

• Clinic checksum is not matching. (Fixed)
• MSVC is failing with a mysterious error Error: D:\a\cpython\cpython\Modules\clinic\_testmultiphase.c.h(98,1): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testmultiphase.vcxproj]. Any ideas? @zooba?

[ericsnowcurrently]

I'll take a look when I get a chance.

[arhadthedev]

MSVC is failing with a mysterious error Error: D:\a\cpython\cpython\Modules\clinic_testmultiphase.c.h(98,1): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild_testmultiphase.vcxproj].

@kumaraditya303 It looks like a missing comma:

-        .ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS)
+        .ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS),

[kumaraditya303]

Error: D:\a\cpython\cpython\Modules\clinic_testmultiphase.c.h(98,1): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild_testmultiphase.vcxproj]

Okay, after some investigation it seems to be a msvc limitation which does not allow accessing address of dynamically loaded libraries.

[kumaraditya303]

I can think of two solutions at this point:

• Statically link that module (hard solution requires changes to build files)
• Remove argument clinic from that module as it is not that much (easier to do).

[zooba]

Yeah, because DLLs get relocated on load, it won't statically embed a pointer into a different DLL. You need to add an assignment in code after everything has been loaded (which is basically anywhere apart from a static initializer that's not in a function).

[zooba]

Looking closer at the code, you might also want to change that module to not bulid with Py_BUILD_CORE, since all the code that assumes it's part of the core runtime module is under that flag.

Alternatively, removing argument clinic use from the module should also fix it.

[kumaraditya303]

Yeah, because DLLs get relocated on load, it won't statically embed a pointer into a different DLL. You need to add an assignment in code after everything has been loaded (which is basically anywhere apart from a static initializer that's not in a function).

Thanks for the info, I wanted to know are there any downsides to statically linking all the extension modules on Windows? Slow startup maybe ?

[zooba]

It will certainly slow down startup when those modules aren't going to be used, as they often reference many more DLLs than are needed by the core. You'd need to convert a whole lot of others into delay loads to make up for it. It also makes it harder to omit functionality (e.g. I can easily make a network-less distro for embedding by leaving out _socket.pyd, or leave out _ctypes.pyd and know that there's no FFI). It also hurts compile time, potentially exponentially (I don't know exactly how link-time optimisation handles it).

[zooba]

We'd certainly want to avoid statically linking test modules into the main build, too.

[gvanrossum]

C2099 is a notorious error, but we have to live with it. IIRC it's the reason so many static type initializers use PyVarObject_HEAD_INIT(NULL, 0) instead of PyVarObject_HEAD_INIT(&PyType_Type, 0) most of the time. Steve's right that the solution is to patch in the correct value at runtime during initialization.

[ericsnowcurrently]

I'm going to tackle per-interpreter kwtuples in a separate issue.