Replace _Py_IDENTIFIER() with statically initialized objects.

@vstinner

BPO	46541
Nosy	@vstinner, @tiran, @ericsnowcurrently, @serhiy-storchaka, @corona10, @erlend-aasland, @kumaraditya303, @notarealdeveloper
PRs	bpo-46541: Replace core use of _Py_IDENTIFIER() with statically initialized global objects. #30928 bpo-46541: Generate the global objects initializer. #30941 bpo-46712: Share global string identifiers in deepfreeze #31261 bpo-46541: Regen the global objects using PYTHON_FOR_REGEN. #31344 bpo-46541: Discover the global strings. #31346 bpo-46541: Replace _Py_IDENTIFIER with _Py_ID in sqlite3 #31351 bpo-46541: Remove usage of _Py_IDENTIFIER from dbms modules #31358 bpo-46541: Drop the c-analyzer check for orphaned global strings. #31363 bpo-46541: Scan Fewer Files in generate_global_objects.py #31364 bpo-46541: Remove usage of _Py_IDENTIFIER from csv module #31372 bpo-46541: Remove usage of _Py_IDENTIFIER from mmap module #31375 bpo-46541: Remove usage of _Py_IDENTIFIER from array module #31376 bpo-46210: Fix deadlock in print. #30310 bpo-46541: Remove usage of _Py_IDENTIFIER from pyexpat #31468 bpo-46541: Remove usage of _Py_IDENTIFIER from multibytecodec #31475 bpo-46541: remove usage of _Py_IDENTIFIER from _ssl module #31599 bpo-46541: Remove unnecessary Py_VISIT (GH-31608) #31608 bpo-46541: Remove unneeded visits from sqlite3 #31609 bpo-46541: Remove usage of _Py_IDENTIFIER from lzma module #31683 bpo-46541: Add a comment about when to use _Py_DECLARE_STR(). #32063

^{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']

PR: GH-90699: Remove _Py_IDENTIFIER usage from _json module #98956

PR: GH-90699: Remove _Py_IDENTIFIER usage from _curses module #98957

PR: GH-90699: Remove _Py_IDENTIFIER usage from _asyncio module #99010

PR: GH-90699: Remove _Py_IDENTIFIER usage from _elementtree module #99012

PR: GH-90699: Remove _Py_IDENTIFIER usage from _ctypes #99054

PR: GH-90699: Remove remaining _Py_IDENTIFIER stdlib usage #99067

PR: GH-90699: disallow _Py_IDENTIFIER in core code #99210

PR: GH-90699: fix refleak in _testcapimodule.c #99236

PR: GH-90699: Clear interned strings in _elementtree #101185

_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).

## 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/python-dev@python.org/message/FRUTTE47JO2XN3LXV2J4VB5A5VILILLA/

New changeset 247480a by Eric Snow in branch 'main':
bpo-46541: Generate the global objects initializer. (gh-30941)
247480a

_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?

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.

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

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?

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.

GH-90699: Remove `_Py_IDENTIFIER` usage from `_ctypes` (GH-99054)

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.

We will address Programs/_testembed.c in the issue/PR where we remove _Py_IDENTIFIER() (or decide to not).

GH-90699: Remove remaining `_Py_IDENTIFIER` stdlib usage (GH-99067)

@kumaraditya303

Thanks for doing all that work, @kumaraditya303!

We can close this once gh-99210 lands.

GH-90699: fix refleak in `_testcapimodule.c` (gh-99236)

GH-90699: disallow `_Py_IDENTIFIER` in core code (GH-99210)

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.

@ericsnowcurrently

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.

pythonGH-90699: Clear interned strings in _elementtree

GH-90699: Clear interned strings in `_elementtree` (#101185)

Navigation Menu

Repository navigation

Replace _Py_IDENTIFIER() with statically initialized objects. #90699

47 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Replace _Py_IDENTIFIER() with statically initialized objects. #90699

Description

Activity

ericsnowcurrently commented on Jan 26, 2022

ericsnowcurrently commented on Jan 26, 2022

ericsnowcurrently commented on Jan 27, 2022

vstinner commented on Jan 28, 2022

ericsnowcurrently commented on Feb 1, 2022

ericsnowcurrently commented on Feb 2, 2022

ericsnowcurrently commented on Feb 4, 2022

ericsnowcurrently commented on Feb 8, 2022

lazka commented on Feb 11, 2022

47 remaining items

kumaraditya303 commented on Nov 3, 2022

kumaraditya303 commented on Nov 3, 2022

kumaraditya303 commented on Nov 7, 2022

ericsnowcurrently commented on Nov 7, 2022

ericsnowcurrently commented on Nov 7, 2022

ericsnowcurrently commented on Nov 7, 2022

ericsnowcurrently commented on Nov 9, 2022

kumaraditya303 commented on Nov 9, 2022

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions