Repository navigation
Replace _Py_IDENTIFIER() with statically initialized objects. #90699
Description
Activity
_Py_Identifierhas been useful but at this point there is a faster and simpler approach we could take as a replacement: statically initialize the objects as fields on_PyRuntimeStateand reference them directly through a macro.This would involve the following:
- add a
PyUnicodeObject field (not a pointer) to_PyRuntimeStatefor each string that currently uses_Py_IDENTIFIER()` - initialize each object as part of the static initializer for
_PyRuntimeState - make each "immortal" (e.g. start with a really high refcount)
- add a macro to look up a given string
- update each location that currently uses
_Py_IDENTIFIER()to use the new macro instead
As part of this, we would also do the following:
- get rid of all C-API functions with
_Py_Identiferparameters - get rid of the old runtime state related to identifiers
- get rid of
_Py_Identifier,_Py_IDENTIFIER(), etc.
(Note that there are several hundred uses of
_Py_IDENTIFIER(), including a number of duplicates.)Pros:
- reduces indirection (and extra calls) for C-API using the strings (making the code easier to understand and speeding it up)
- the objects are referenced from a fixed address in the static data section (speeding things up and allowing the C compiler to optimize better)
- there is no lazy allocation (or lookup, etc.) so there are fewer possible failures when the objects get used (thus less error return checking)
- simplifies the runtime state
- saves memory (at little, at least)
- the approach for per-interpreter is simpler (if needed)
- reduces the number of static variables in any given C module
- reduces the number of functions in the ("private") C-API
- "deep frozen" modules can use these strings
- other commonly-used strings could be pre-allocated by adding
_PyRuntimeStatefields for them
Cons:
- churn
- adding a string to the list requires modifying a separate file from the one where you actually want to use the string
- strings can get "orphaned" (we could prevent this with a check in
make check) - some PyPI packages may rely on
_Py_IDENTIFIER()(even though it is "private" C-API) - some strings may never get used for any given ./python invocation
Note that with a basic partial implementation (GH-30928) I'm seeing a 1% improvement in performance (see faster-cpython/ideas#230).
- add a
## Background ##
_Py_Identifier(and_Py_IDENTIFIER(), etc.) was added in 2011 [1][2] for several reasons:- provide a consistent approach for a common optimization: caching C-string based string objects
- facilitate freeing those objects during runtime finalization
The solution involved using a static variable defined, using
_Py_IDENTIFIER()near the code that needed the string. The variable (a_Py_Identifier) would hold the desired C string (statically initialized) and the corresponding (lazily created)PyUnicodeObject. The code where the_Py_Identifierwas defined would then pass it to specialized versions of various C-API that would normally consume a C string orPyUnicodeObject. Then that code would use either the C-string or the object (creating and caching it first if not done already). This approach decentralized the caching but also provided a single tracking mechanism that made it easier to clean up the objects.Over the last decade a number of changes were made, including recent changes to make the identifiers per-interpreter and to use a centralized cache.
[1] afe55bb
[2] https://mail.python.org/archives/list/python-dev@python.org/message/FRUTTE47JO2XN3LXV2J4VB5A5VILILLA/_Py_Identifierhas been useful but at this point there is a faster and simpler approach we could take as a replacement: statically initialize the objects as fields on_PyRuntimeStateand reference them directly through a macro.This change is going to break projects in the wild. Yes, people use the _Py_IDENTIFIER(), _PyUnicode_FromId() and other "Id" variant of many functions in 3rd party projects.
Is it possible to keep runtime initialization if this API is used by 3rd party code?
If necessary, we can keep _Py_IDENTIFIER() (and the functions). Regardless, we can stop using it internally.
FYI, I've posted to python-dev for feedback before proceeding:
https://mail.python.org/archives/list/python-dev@python.org/thread/DNMZAMB4M6RVR76RDZMUK2WRLI6KAAYS/
(thanks Victor: https://mail.python.org/archives/list/python-dev@python.org/message/7RMLIJHUWVBZFV747TFEHOE6LNBVQSMM/)
3rd party use of _Py_IDENTIFIER():
- blender
+ https://github.com/blender/blender/blob/master/source/blender/python/intern/bpy_traceback.c#L53
- copied from core code
- "msg", "filename", "lineno", "offset", "text", "<string>"
- uses _PyObject_GetAttrId() - datatable
+ https://github.com/h2oai/datatable/blob/45a87337bc68576c7fb6900f524925d4fb77d6a6/src/core/python/obj.cc#L76
- in C++ wrapper getting sys.stdout, etc. and writing to sys.stdout
- has to hack around C++14 support
- has a fallback under limited API
- "stdin", "stdout", "stderr", "write"
- uses _PySys_GetObjectId(), _PyObject_GetAttrId() - multidict (in aiohttp)
+ https://github.com/aio-libs/multidict/blob/6dedb623cca8e8fe64f502dfa479826efc321385/multidict/_multilib/defs.h#L8
+ https://github.com/aio-libs/multidict/blob/6dedb623cca8e8fe64f502dfa479826efc321385/multidict/_multilib/istr.h#L46
+ https://github.com/aio-libs/multidict/blob/6dedb623cca8e8fe64f502dfa479826efc321385/multidict/_multilib/pair_list.h#L114
- calling str.lower()
- "lower"
- uses _PyObject_CallMethodId() - mypy (exclusively in mypyc, including in generated code!)
+ https://github.com/python/mypy/blob/3c935bdd1332672f5daeae7f3f9a858a453333d4/mypyc/lib-rt/dict_ops.c#L76
+ https://github.com/python/mypy/blob/3c935bdd1332672f5daeae7f3f9a858a453333d4/mypyc/lib-rt/dict_ops.c#L131
- "setdefault", "update"
- uses _PyObject_CallMethodIdObjArgs(), _PyObject_CallMethodIdOneArg()
+ https://github.com/python/mypy/blob/2b7e2df923f7e4a3a199915b3c8563f45bc69dfa/mypyc/lib-rt/pythonsupport.h#L26
+ https://github.com/python/mypy/blob/2b7e2df923f7e4a3a199915b3c8563f45bc69dfa/mypyc/lib-rt/pythonsupport.h#L109
- "__mro_entries__", "__init_subclass__"
- uses _PyObject_LookupAttrId(), _PyObject_GetAttrId()
+ https://github.com/python/mypy/blob/2b7e2df923f7e4a3a199915b3c8563f45bc69dfa/mypyc/lib-rt/misc_ops.c#L27
+ https://github.com/python/mypy/blob/2b7e2df923f7e4a3a199915b3c8563f45bc69dfa/mypyc/lib-rt/misc_ops.c#L47
- "send", "throw", "close"
- uses _PyObject_CallMethodIdOneArg(), _PyObject_GetAttrId()
+ https://github.com/python/mypy/blob/8c5c915a89ec0f35b3e07332c7090e62f143043e/mypyc/lib-rt/bytes_ops.c#L104
- "join"
- uses _PyObject_CallMethodIdOneArg()
+ https://github.com/python/mypy/blob/3c935bdd1332672f5daeae7f3f9a858a453333d4/mypyc/codegen/emitwrapper.py#L326
+ https://github.com/python/mypy/blob/2b7e2df923f7e4a3a199915b3c8563f45bc69dfa/mypyc/lib-rt/misc_ops.c#L694
- uses _PyObject_GetAttrId() - pickle5
+ https://github.com/pitrou/pickle5-backport/blob/e6117502435aba2901585cc6c692fb9582545f08/pickle5/_pickle.c#L224
+ https://github.com/pitrou/pickle5-backport/blob/e6117502435aba2901585cc6c692fb9582545f08/pickle5/compat.h
- "getattr"
- uses _PyUnicode_FromId() - pysqlite3
+ https://github.com/coleifer/pysqlite3/blob/f302859dc9ddb47a1089324dbca3873740b74af9/src/microprotocols.c#L103
+ https://github.com/coleifer/pysqlite3/blob/f302859dc9ddb47a1089324dbca3873740b74af9/src/microprotocols.c#L119
- "__adapt__", "__conform__"
- uses _PyObject_CallMethodId()
+ https://github.com/coleifer/pysqlite3/blob/093b88d1a58b141db8cf971c35ea1f6b674d0d02/src/connection.c#L51
+ https://github.com/coleifer/pysqlite3/blob/093b88d1a58b141db8cf971c35ea1f6b674d0d02/src/connection.c#L772
- "finalize", "value", "upper", "cursor"
- uses _PyObject_CallMethodId(), _PyObject_CallMethodIdObjArgs()
+ https://github.com/coleifer/pysqlite3/blob/49ce9c7a89a3c9f47ab8d32b6c4e2f7d629c1688/src/module.c#L195
- "upper"
- uses _PyObject_CallMethodId()
+ https://github.com/coleifer/pysqlite3/blob/91b2664f525b19feedfca3f0913302c6f1e8be8a/src/cursor.c#L103
- "upper"
- uses _PyObject_CallMethodId() - typed_ast
+ a fork of CPython's ast code - zodbpickle
+ a fork of CPython's pickle
All of them should be trivial to drop _Py_IDENTIFIER() without any real performance impact or mess.
Also, the following implies that PyPy has some sort of _Py_IDENTIFIER() support: https://github.com/benhoyt/scandir/blob/3396aa4155ffde8600a0e9ca50d5872569169b5d/_scandir.c#L51.
- blender
lazka commented
on Feb 11, 2022 lazkamannequinMannequinSorry if off topic, but I noticed that CPython doesn't deprecate macros in code, while with gcc/clang it's possible to show compiler warnings for them using some pragma magic:
$ gcc a.c a.c: In function 'main': a.c:29:13: warning: Deprecated pre-processor symbol 29 | PySomethingDeprecated (0); | ^~~~~~~~~~~~~~~~~~ a.c:30:13: warning: Deprecated pre-processor symbol: replace with "SomethingCompletelyDifferent" 30 | PySomethingDeprecated2 (42); | ^~~~~~~~~~~~~~~~~~~~
Here is how glib implements this for example: https://gist.github.com/lazka/4749c74249a3918a059d944040aca4a3
Maybe that makes getting rid of them easier in the long run?
47 remaining items
I'm okay with that, though ideally we would use public API as much as possible and limit _Py_ID() to performance-sensitive code. That said, I favor getting this done over being excessively careful about how much we use _Py_ID(). 🙂
practicality beats purity :) We can of course reconsider this when these modules will be ported to multiphase heap types.
PR #99067 removes the remaining.
FYI, once #99067 is merged, I'll disable using
_Py_IDENTIFIERin core code. With that completed I consider this done and deprecation/removal for_Py_IDENTIFIERcan be discussed separately.Reacted by Eric Snow and Erlend E. AaslandWe will address Programs/_testembed.c in the issue/PR where we remove
_Py_IDENTIFIER()(or decide to not).Reacted by Kumar AdityaThanks for doing all that work, @kumaraditya303!
Reacted by Erlend E. Aasland and Kumar AdityaWe can close this once gh-99210 lands.
- added a commit that references this issue
on Nov 8, 2022 This side of things is done. We can create a new issue about maybe getting rid of
_Py_IDENTIFIER()and what to do about third-party usage in the community.Reacted by Erlend E. Aasland and Kumar AdityaThanks to everyone!
@ericsnowcurrently: It would be nice to update the c analyzer tool as a lot of globals are removed as part of this.
Reacted by Eric Snow and Erlend E. Aasland- added a commit that references this issue
on Jan 20, 2023
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
_Py_IDENTIFIERusage from_jsonmodule #98956_Py_IDENTIFIERusage from_cursesmodule #98957_Py_IDENTIFIERusage from_asynciomodule #99010_Py_IDENTIFIERusage from_elementtreemodule #99012_Py_IDENTIFIERusage from_ctypes#99054_Py_IDENTIFIERstdlib usage #99067_Py_IDENTIFIERin core code #99210_testcapimodule.c#99236