homepage

Both https://github.com/python/cpython/pull/18066 (collections module) and https://github.com/python/cpython/pull/18032 (asyncio module) ran into the problem where porting them to multi-phase initialisation involves replacing their usage of the `_Py_IDENTIFIER` macro with some other mechanism.
When _posixsubprocess was ported, the replacement was a relatively ad hoc combination of string interning and the interpreter-managed module-specific state: https://github.com/python/cpython/commit/5a7d2e11aaea2dd32878dc5c6b1aae8caf56cb44
I'm wondering if we may able to devise a comparable struct-field based system that replaces the `_Py_IDENTIFIER` local static variable declaration macro and the `Py_Id_<name>` lookup convention with a combination like (using the posix subprocess module conversion as an example):
 // Identifier usage declaration (replaces _Py_IDENTIFIER)
 _Py_USE_CACHED_IDENTIFIER(_posixsubprocessstate(m), disable);
 // Identifier usage remains unchanged, but uses a regular local variable
 // rather than the static variable declared by _Py_IDENTIFIER
 result = _PyObject_CallMethodIdNoArgs(gc_module, &PyId_disable);
And then the following additional state management macros would be needed to handle the string interning and reference counting:
 // Module state struct declaration
 typedef struct {
 // This would declare an initialised array of _Py_Identifier structs
 // under a name like __cached_identifiers__. The end of the array
 // would be indicated by a strict with "value" set to NULL.
 _Py_START_CACHED_IDENTIFIERS;
 _Py_CACHED_IDENTIFIER(disable);
 _Py_CACHED_IDENTIFIER(enable);
 _Py_CACHED_IDENTIFIER(isenabled);
 _Py_END_CACHED_IDENTIFIERS;
 );
 } _posixsubprocessstate;
 // Module tp_traverse implementation
 _Py_VISIT_CACHED_IDENTIFIERS(_posixsubprocessstate(m));
 // Module tp_clear implementation (also called by tp_free)
 _Py_CLEAR_CACHED_IDENTIFIERS(_posixsubprocessstate(m));
With the requirement to declare usage of the cached identifiers, they could be lazily initialized the same way the existing static variables are (even re-using the same struct declaration).
Note: this is just a draft of one possible design, the intent of this issue is to highlight the fact that this issue has now come up multiple times, and it would be good to have a standard answer available.

Once I discussed with Eric Snow during a core developer sprint: _Py_IDENTIFIER() should use an "interpreter local storage" for identifiers values. _Py_IDENTIFIER() would only be a "key" and _PyUnicode_FromId() would store the value somewhere in a hash table stored in PyInterpreterState. Something similar to the TSS API:
* PyThread_create_key()
* PyThread_delete_key_value()
* PyThread_set_key_value()
* PyThread_get_key_value()
But per interpreter, rather than being per thread.
The key can be simply the variable address in memory. It only has to be unique in the process.

> Both https://github.com/python/cpython/pull/18066 (collections module) and https://github.com/python/cpython/pull/18032 (asyncio module) ran into the problem where porting them to multi-phase initialisation involves replacing their usage of the `_Py_IDENTIFIER` macro with some other mechanism.
What is the problem between _Py_IDENTIFIER and multi-phase initialisation modules?
If both are incompatible, we may need a different but similar API: values would be stored in a hash table per module object. The hash table can be stored in the module object directly, or it can be store in a second hash table (module => hash table).
If we want a unified API, maybe we can use module=NULL (or any other marker) for "global" identifiers (not specific to a module).

> What is the problem between _Py_IDENTIFIER and multi-phase initialisation modules?
AFAIK there is no problem now, except possibly a race condition when initializing the identifiers.
It seems it's too easy to conflate porting to multi-phase initialization and getting rid of static state.
The problem will come with per-interpreter reference counting, or when the `str` class is no longer shared across all interpreters. For that, we'll need either per-interpreter identifiers, or solve the issue in another way.

> AFAIK there is no problem now, except possibly a race condition when initializing the identifiers.
The GIL avoids any risk of race condition, no?

> The GIL avoids any risk of race condition, no?
Looks like the GIL would affect performance more or less?
>_Py_IDENTIFIER() would only be a "key" and _PyUnicode_FromId() would >store the value somewhere in a hash table stored in PyInterpreterState.
+1.
IMHO, for those two cases, the simplest idea is move IDENTIFIER to moduleState which would increase more memory usage than InterpreterState.

As Petr notes, as long as all subinterpreters share the GIL, and share str instances, then the existing _Py_IDENTIFIER mechanism will work fine for both single phase and multi-phase initialisation.
However, that constraint also goes the other way: as long as we have modules that use the existing _Py_IDENTIFIER mechanism, then subinterpreters *must* share str instances, and hence *must* share the GIL.
Hence the "enhancement" classification: there's nothing broken right now, but if we're ever going to achieve the design goal of using subinterpreters to exploit multiple CPU cores without the overhead of running multiple full interpreter processes, we're going to need to design a different way of handling this.
Something to keep in mind with `_Py_IDENTIFIER` and any replacement API: the baseline for performance comparisons is https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_InternFromString
The reason multi-phase initialisation makes this more complicated is that it means we can't use the memory addresses of C process globals as unique identifiers any more, since more than one module object may be created from the same C shared library.
However, if we assume we've moved to per-module state storage (to get unique memory addresses back), then we can largely re-use the existing `_Py_IDENTIFIER` machinery to make the lookup as fast as possible, while still avoiding conflicts between subinterpreters.

New changeset 4804b5b3df82e7892ca0550b02f902bcfc16bb48 by Victor Stinner in branch 'master':
bpo-39465: Don't access directly _Py_Identifier members (GH-20043)
https://github.com/python/cpython/commit/4804b5b3df82e7892ca0550b02f902bcfc16bb48

I created bpo-40602: "Move Modules/hashtable.h to Include/internal/pycore_hashtable.h".

Attached bench.py: Micro-benchmark on _PyUnicode_FromId(). It requires attached bench.patch being applied.

New changeset d6fb53fe42d83a10f1372dd92ffaa6a01d2feffb by Victor Stinner in branch 'master':
bpo-39465: Remove _PyUnicode_ClearStaticStrings() from C API (GH-20078)
https://github.com/python/cpython/commit/d6fb53fe42d83a10f1372dd92ffaa6a01d2feffb

New changeset 297257f7bc198e2dc8e0866b539c73ff1a5cc588 by Victor Stinner in branch 'master':
bpo-39465: Cleanup _PyUnicode_FromId() code (GH-20595)
https://github.com/python/cpython/commit/297257f7bc198e2dc8e0866b539c73ff1a5cc588

New changeset 1bcc32f0620d2e99649a6d423284d9496b7b3548 by Victor Stinner in branch 'master':
bpo-39465: Use _PyInterpreterState_GET() (GH-20788)
https://github.com/python/cpython/commit/1bcc32f0620d2e99649a6d423284d9496b7b3548

New changeset 52a327c1cbb86c7f2f5c460645889b23615261bf by Victor Stinner in branch 'master':
bpo-39465: Add pycore_atomic_funcs.h header (GH-20766)
https://github.com/python/cpython/commit/52a327c1cbb86c7f2f5c460645889b23615261bf

New changeset ba3d67c2fb04a7842741b1b6da5d67f22c579f33 by Victor Stinner in branch 'master':
bpo-39465: Fix _PyUnicode_FromId() for subinterpreters (GH-20058)
https://github.com/python/cpython/commit/ba3d67c2fb04a7842741b1b6da5d67f22c579f33

Ok, it should now be fixed. I close the issue.
See PR 20085 "Per-interpreter interned strings" of bpo-40521 for the follow-up.

This change introduced a subtle regression: bpo-46006 "[subinterpreter] _PyUnicode_EqualToASCIIId() issue with subinterpreters".