bpo-46006: Move the interned strings and identifiers to _PyRuntimeState.#30131
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
I didn't review the code but this seems like a good way to fix bpo-46006. We can have more time to figure out how to make them per-interpreter (assuming that's the decision). |
| /* Unicode identifiers (_Py_Identifier): see _PyUnicode_FromId() */ | ||
| struct_Py_unicode_ids{ | ||
| PyThread_type_locklock; |
There was a problem hiding this comment.
Isn't this lock unnecessary? The GIL is held whenever an identifier is used, isn't it?
| Another way to look at this is that to say that the actual reference | ||
| count of a string is: s->ob_refcnt + (s->state ? 2 : 0) | ||
| */ | ||
| PyObject*unicode_interned; |
There was a problem hiding this comment.
Could you use "string" or "str" rather than "unicode". Python 2 is history 🙂
| count of a string is: s->ob_refcnt + (s->state ? 2 : 0) | ||
| */ | ||
| PyObject*unicode_interned; | ||
| } cached; |
| #endif | ||
| #defineIDENTIFIERS _Py_SINGLETON(unicode_ids) |
There was a problem hiding this comment.
Can you drop the IDENTIFIERS and INTERNED macros. They impair readability.
You can leave _Py_SINGLETON as it conveys some meaning.
| struct_Py_unicode_runtime_ids*rt_ids=&interp->runtime->unicode_ids; | ||
| PyThread_acquire_lock(rt_ids->lock, WAIT_LOCK); | ||
| PyThread_acquire_lock(IDENTIFIERS.lock, WAIT_LOCK); |
There was a problem hiding this comment.
Drop this, and assert that the GIL is held?
markshannon commented Jan 5, 2022
I agree with @nascheme that this is probably the best fix for 3.11a4. Let's hope that we can keep the overhead of immortal objects low enough that we can have parallel sub-interpreters and static global objects. |
vstinner commented Jan 5, 2022
https://bugs.python.org/issue46006 is about interned strings. Fixing this issue doesn't need to share again identifiers between all interpreters. I'm not convinced by the value of moving the structures from PyInterpreterState to _PyRuntimeState. I propose a different PR which only reverts ea25180 : PR #30422. It only moves the |
vstinner commented Jan 6, 2022
I merged my GH-20085 revert. |
6 participants
Currently the interned strings (and strings created for
_Py_IDENTIFIER()) are per-interpreter. This is causing some bugs because other objects which may hold a reference to the string are still global. So until we are closer to moving the bulk of the global objects to per-interpreter, the simplest thing is to move the interned strings (and identifiers) to_PyRuntimeState.https://bugs.python.org/issue46006