classification
Title: Design a subinterpreter friendly alternative to _Py_IDENTIFIER
Type: enhancement Stage:
Components: Versions:
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, ncoghlan, petr.viktorin, shihai1991, vstinner
Priority: normal Keywords:

Created on 2020-01-27 14:28 by ncoghlan, last changed 2020-02-08 12:52 by ncoghlan.

Messages (7)
msg360766 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2020-01-27 14:28
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.
msg361041 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-30 11:35
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.
msg361042 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-30 11:41
> 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).
msg361044 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2020-01-30 11:53
> 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.
msg361045 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-30 11:54
> AFAIK there is no problem now, except possibly a race condition when initializing the identifiers.

The GIL avoids any risk of race condition, no?
msg361533 - (view) Author: hai shi (shihai1991) * Date: 2020-02-07 02:59
> 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.
msg361627 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2020-02-08 12:52
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.
History
Date User Action Args
2020-02-08 12:52:39ncoghlansetmessages: + msg361627
2020-02-07 02:59:23shihai1991setmessages: + msg361533
2020-01-30 11:54:57vstinnersetmessages: + msg361045
2020-01-30 11:53:21petr.viktorinsetmessages: + msg361044
2020-01-30 11:41:22vstinnersetmessages: + msg361042
2020-01-30 11:35:23vstinnersetnosy: + vstinner
messages: + msg361041
2020-01-27 14:28:51ncoghlansettype: enhancement
2020-01-27 14:28:30ncoghlancreate