GeezerVison is showing github.com — [subinterpreter] Make type version tag counter threadsafe for per interpreter GIL · Issue #95795 · python/cpython
[subinterpreter] Make type version tag counter threadsafe for per interpreter GIL #95795DescriptionThe next_version_tag in typeobject.c is currently not thread safe and relies on GIL to protect against concurrent increments across threads.
|
// bpo-42745: next_version_tag remains shared by all interpreters because of static types |
|
// Used to set PyTypeObject.tp_version_tag |
|
static unsigned int next_version_tag = 1; |
|
|
|
typedef struct PySlot_Offset { |
For per-interpreter GIL, it must be made thread-safe otherwise the type cache will be affected by race conditions. Static types are not affected because they are immutable so this is a issue for pure Python classes aka heap types. This issue is for discussion of possible implementations.
Possible Solutions:
- Make the
next_version_tag counter per interpreter, while this may seems to fix the issue but since objects are shared across the interpreters and the type version tag is also used to represent a current state of a type in the specializing interpreter, two interpreters can have same the same tag for different types this won't work as expected.
- Make the
next_version_tag an atomic counter and increment it atomically using the pyatomic APIs. This is my preferred solution since next_version_tag is only modified when a type is modified so is a rare operation and not a performance issue. Since this is just a counter relaxed ordering can be used here.
cc @ericsnowcurrently
Linked PRs
Activityericsnowcurrently commented on Aug 9, 2022 ericsnowcurrently commented on Aug 9, 2022 Oh, I misread that as tp_version_tag. 😄
We should definitely fix next_version_tag. ericsnowcurrently commented on Aug 9, 2022 FTR, the comment in the embedded code snippet above refers to gh-86911. ericsnowcurrently commented on Aug 9, 2022 ericsnowcurrently commented on Aug 9, 2022 As for me...
Make the next_version_tag counter per interpreter, while this may seems to fix the issue but since objects are shared across the interpreters and the type version tag is also used to represent a current state of a type in the specializing interpreter, two interpreters can have same the same tag for different types this won't work as expected.
Yeah, it would depend on the specialization semantics.
Also, the reference to static types in the snippet you pasted above implies a special relationship with those objects that we may need to revisit. For static types (especially builtin ones) we don't expect them to change after PyType_Ready() is called. So we wouldn't expect them to ever have tp_version_tag modified and therefore next_version_tag is, in practice, only used for heap types. tp_version_tag is updated when methods get cached but those are cached on each interpreter, so we can use a per-interpreter next_version_tag.
(This does imply that we may need to make tp_version_tag per-interpreter for static builtin types.)
Make the next_version_tag an atomic counter and increment it atomically using the pyatomic APIs. This is my preferred solution since next_version_tag is only modified when a type is modified so is a rare operation and not a performance issue. Since this is just a counter relaxed ordering can be used here.
Yeah, this may make sense. Ideally we could make this per-interpreter but using an atomic seems like a reasonable alternative. markshannon commented on Aug 9, 2022 What I would prefer, is for immutable, sharable classes to set their tag during initialization, or even statically.
Then mutable classes to use a per-interpreter tag.
The global version tags should be in range(0, 1<<16) and the per-interpreter tags should use values above that.
We will want the most common types to have pre-defined tags, for rapidly getting type information from version numbers in the higher tier optimizers. markshannon commented on Aug 9, 2022 Currently there are under 100 Py_TPFLAGS_IMMUTABLETYPE flagged classes in CPython, so 64k should be enough tags. ericsnowcurrently commented on Aug 10, 2022 So we would do the following:
- add
#define _Py_MAX_GLOBAL_TYPE_VERSION_TAG ((2<<16) - 1) in pycore_typeobject.h
- handle static builtin types
- move
next_version_tag to _PyRuntimeState.types.next_version_tag
- set it to 1 in the static initializer (in pycore_runtime_init.h)
- in
_PyStaticType_InitBuiltin(), set tp_version_tag to the next version tag (and add Py_TPFLAGS_VALID_VERSION_TAG to tp_flags)
- add a check for
_Py_Runtime.types.next_version_tag <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG in _PyStaticType_InitBuiltin()
- handle all other types
- add
PyInterpreterState.types.next_version_tag
- set it to
_Py_MAX_GLOBAL_TYPE_VERSION_TAG + 1 in the static initializer (in pycore_runtime_init.h)
- update
_PyType_ClearCache() (typeobject.c) to use the per-interpreter variable
- update
assign_version_tag() (typeobject.c) to use the per-interpreter variable
At that point we could drop next_version_tag in typeobject.c.
There are a few other things to consider:
- add various asserts to ensure code that modifies
tp_version_tag never touches static builtin types?
- should we treat non-builtin static types the same as the builtin ones?
That last one would require some extra work, e.g. special-casing non-heap types to use _Py_Runtime.types.next_version_tag in PyType_Ready() (and probably drop the code from _PyStaticType_InitBuiltin()), as well as adding a global lock there (or making _Py_Runtime.types.next_version_tag atomic). My inclination is to not bother if we can help it. In the end, non-builtin static types will only exist in extension modules that won't be used in multiple interpreters, so there shouldn't be a need to treat their tp_version_tag specially. markshannon commented on Aug 11, 2022 That sounds about right. I'm not familiar with all the static initialization logic, but as long the following assertions always hold, I'm happy:
assert((tp->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) == 0 ||
(tp->tp_version_tag <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG && tp->tp_version_tag));
assert((tp->tp_flags &Py_TPFLAGS_IMMUTABLETYPE) ||
(tp->tp_version_tag == 0 || tp->tp_version_tag > _Py_MAX_GLOBAL_TYPE_VERSION_TAG));
I'd give the two next_version_tags different names.
add various asserts to ensure code that modifies tp_version_tag never touches static builtin types?
More asserts is good.
should we treat non-builtin static types the same as the builtin ones?
Whether they are static should irrelevant. But if the are immutable and can be shared, then they must have their tp_version_tag set with a process-wide lock held, and should be before they are visible to any interpreter. If then can't be shared, then no need for special treatment. Reacted by Eric Snow, Kumar Aditya and Erlend E. Aasland
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|