GeezerVison is showing github.com — Replace _Py_IDENTIFIER() with statically initialized objects. · Issue #90699 · python/cpython
Replace _Py_IDENTIFIER() with statically initialized objects. #90699Description
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:
assignee = 'https://github.com/ericsnowcurrently'
closed_at = None
created_at = <Date 2022-01-26.22:01:04.421>
labels = ['interpreter-core', '3.11']
title = 'Replace _Py_IDENTIFIER() with statically initialized objects.'
updated_at = <Date 2022-03-23.15:52:58.371>
user = 'https://github.com/ericsnowcurrently' bugs.python.org fields:
activity = <Date 2022-03-23.15:52:58.371>
actor = 'eric.snow'
assignee = 'eric.snow'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2022-01-26.22:01:04.421>
creator = 'eric.snow'
dependencies = []
files = []
hgrepos = []
issue_num = 46541
keywords = ['patch']
message_count = 28.0
messages = ['411799', '411800', '411910', '411987', '412229', '412405', '412488', '412868', '413047', '413086', '413246', '413274', '413311', '413312', '413323', '413333', '413334', '413337', '413355', '413374', '413384', '413653', '414178', '414182', '414265', '414274', '414534', '415881']
nosy_count = 8.0
nosy_names = ['vstinner', 'christian.heimes', 'eric.snow', 'serhiy.storchaka', 'corona10', 'erlendaasland', 'kumaraditya', 'notarealdeveloper']
pr_nums = ['30928', '30941', '31261', '31344', '31346', '31351', '31358', '31363', '31364', '31372', '31375', '31376', '30310', '31468', '31475', '31599', '31608', '31609', '31683', '32063']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue46541'
versions = ['Python 3.11'] Activityericsnowcurrently commented on Jan 26, 2022 _Py_Identifier has 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 _PyRuntimeState and 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_Identifer parameters
- 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
_PyRuntimeState fields 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). ericsnowcurrently commented on Jan 26, 2022 ## 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_Identifier was defined would then pass it to specialized versions of various C-API that would normally consume a C string or PyUnicodeObject. 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/[email protected]/message/FRUTTE47JO2XN3LXV2J4VB5A5VILILLA/ ericsnowcurrently commented on Jan 27, 2022 vstinner commented on Jan 28, 2022
_Py_Identifier has 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 _PyRuntimeState and 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? ericsnowcurrently commented on Feb 1, 2022 If necessary, we can keep _Py_IDENTIFIER() (and the functions). Regardless, we can stop using it internally. ericsnowcurrently commented on Feb 2, 2022 ericsnowcurrently commented on Feb 4, 2022 (thanks Victor: https://mail.python.org/archives/list/[email protected]/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. ericsnowcurrently commented on Feb 8, 2022 New changeset 81c7204 by Eric Snow in branch 'main':
bpo-46541: Replace core use of _Py_IDENTIFIER() with statically initialized global objects. (gh-30928)
81c7204 lazka commented on Feb 11, 2022 Sorry 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? kumaraditya303 commented on Nov 3, 2022
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. kumaraditya303 commented on Nov 3, 2022 PR #99067 removes the remaining. kumaraditya303 commented on Nov 7, 2022 FYI, once #99067 is merged, I'll disable using _Py_IDENTIFIER in core code. With that completed I consider this done and deprecation/removal for _Py_IDENTIFIER can be discussed separately. Reacted by Eric Snow and Erlend E. Aasland ericsnowcurrently commented on Nov 7, 2022 We will address Programs/_testembed.c in the issue/PR where we remove _Py_IDENTIFIER() (or decide to not). ericsnowcurrently commented on Nov 7, 2022 Reacted by Erlend E. Aasland and Kumar Aditya ericsnowcurrently commented on Nov 7, 2022 ericsnowcurrently commented on Nov 9, 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 Aditya kumaraditya303 commented on Nov 9, 2022 Thanks 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
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|