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?

[ericsnowcurrently]

On Fri, Feb 11, 2022 at 1:36 AM Christoph Reiter <report@bugs.python.org> wrote:

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:
[snip]
Maybe that makes getting rid of them easier in the long run?

That's a good question. We do have Py_DEPRECATED() (in
Include/pyport.h), which is used for symbols. I'm not sure anyone has
given much thought to deprecating macros, but it's probably worth
considering. I recommend that you post something about this to
python-dev@python.org.

FWIW, here are other explanations of how to deprecate macros:

• https://stackoverflow.com/questions/57478368/what-is-the-best-way-to-mark-macro-as-deprecated/57479189#57479189
• https://stackoverflow.com/questions/2681259/how-to-deprecate-a-c-pre-processor-macro-in-gcc/29297970#29297970

[ericsnowcurrently]

With core code sorted out, stdlib and 3rd party extension modules are left to sort out.

I see the following possibilities:

1. leave _Py_IDENTIFIER() alone (it is already disallowed in core code)
2. change _Py_IDENTIFIER() to create static string objects (then get rid of global state)
3. get rid of _Py_IDENTIFIER()
   a. provide a public alternative (needs a PEP)
   b. first work with 3rd party projects to stop using it

[ericsnowcurrently]

New changeset 12360aa by Eric Snow in branch 'main':
bpo-46541: Discover the global strings. (gh-31346)
12360aa

[ericsnowcurrently]

New changeset 6c89589 by Eric Snow in branch 'main':
bpo-46541: Drop the check for orphaned global strings. (gh-31363)
6c89589

[ericsnowcurrently]

New changeset 4d8a515 by Eric Snow in branch 'main':
bpo-46541: Scan Fewer Files in generate_global_objects.py (gh-31364)
4d8a515

[corona10]

New changeset e59309b by Dong-hee Na in branch 'main':
bpo-46541: Remove usage of _Py_IDENTIFIER from dbms modules (GH-31358)
e59309b

[corona10]

New changeset d64f3ca by Dong-hee Na in branch 'main':
bpo-46541: Remove usage of _Py_IDENTIFIER from csv module (GH-31372)
d64f3ca

[corona10]

New changeset b207711 by Erlend Egeberg Aasland in branch 'main':
bpo-46541: Replace _Py_IDENTIFIER with _Py_ID in sqlite3 (GH-31351)
b207711

[corona10]

New changeset e8a19b0 by Dong-hee Na in branch 'main':
bpo-46541: Remove usage of _Py_IDENTIFIER from mmap module (GH-31375)
e8a19b0

[ericsnowcurrently]

(from #31376 (comment))

[corona10]

Should we create the separate bpo issue if module changes are too noisy?

I think it's fine to use the one issue. There are only 26 modules with NEEDS_PY_IDENTIFIER and none have much risk with the change. So it's unlikely that we'll get any discussion about any specific module. If we do, we can easily create an issue then for the module in question. If one of the modules had many uses of _Py_IDENTIFIER() then it might make sense to split it out, but IIRC none of them have very many.

[corona10]

I think it's fine to use the one issue.

Great! and thank you for all your efforts :)

[corona10]

New changeset 8cb5f70 by Dong-hee Na in branch 'main':
bpo-46541: Remove usage of _Py_IDENTIFIER from array module (GH-31376)
8cb5f70

[corona10]

New changeset 2b86616 by Dong-hee Na in branch 'main':
bpo-46541: Remove usage of _Py_IDENTIFIER from pyexpat (GH-31468)
2b86616

[tiran]

New changeset 088dd76 by Dong-hee Na in branch 'main':
bpo-46541: Remove unnecessary Py_VISIT (GH-31608)
088dd76

[corona10]

New changeset c32aef4 by Erlend Egeberg Aasland in branch 'main':
bpo-46541: Remove unneeded visits from sqlite3 (GH-31609)
c32aef4

[kumaraditya303]

ctypes and datetime are similar cases which already use internal C API so I'll change it to use _Py_ID.

[ericsnowcurrently]

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

[kumaraditya303]

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]

PR #99067 removes the remaining.

[kumaraditya303]

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.

[ericsnowcurrently]

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

[ericsnowcurrently]

Thanks for doing all that work, @kumaraditya303!

[ericsnowcurrently]

We can close this once gh-99210 lands.

[ericsnowcurrently]

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.

[kumaraditya303]

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.