Replace _Py_IDENTIFIER() with statically initialized objects.

[ericsnowcurrently]

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

[ericsnowcurrently]

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

## 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/

[ericsnowcurrently]

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

[vstinner]

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

If necessary, we can keep _Py_IDENTIFIER() (and the functions). Regardless, we can stop using it internally.

[ericsnowcurrently]

FYI, I've posted to python-dev for feedback before proceeding:

https://mail.python.org/archives/list/python-dev@python.org/thread/DNMZAMB4M6RVR76RDZMUK2WRLI6KAAYS/

[ericsnowcurrently]

(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.

[ericsnowcurrently]

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]

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?