Repository navigation
_PyStaticType_Dealloc does not invalidate type version tag #101696
Description
Activity
I'll file PR shortly.
Reacted by Erlend E. AaslandGood job debugging that! 👏🏻👏🏻👏🏻
- added a commit that references this issue
on Feb 8, 2023 - changed the title
[-]`_PyStaticType_Dealloc` does not invalidates type version tag[/-][+]`_PyStaticType_Dealloc` does not invalidate type version tag[/+]on Feb 9, 2023 Given that static types should be immutable and immortal, I think a better approach would be reserve low values (up to a 1000 or so) for static classes. Once allocated, a static type would keep the same version number for the lifetime of the process.
It might be handy to pre-allocate a few values for the most common types, but that's not necessary for correctness.
Reacted by Erlend E. AaslandGiven that static types should be immutable and immortal, I think a better approach would be reserve low values (up to a 1000 or so) for static classes. Once allocated, a static type would keep the same version number for the lifetime of the process. It might be handy to pre-allocate a few values for the most common types, but that's not necessary for correctness.
Yeah, it makes sense and is on my todo list for a long time See #95795
Reacted by Eric Snow and Erlend E. AaslandMakes sense to me as well. As a proof-of-concept, reverting #101697 and applying the following seems to work:
Details
diff --git a/Objects/typeobject.c b/Objects/typeobject.c index bf6ccdb77a..bc48845300 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -229,6 +229,13 @@ _PyType_CheckConsistency(PyTypeObject *type) CHECK(type->tp_traverse != NULL); } + if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { + CHECK(type->tp_version_tag != 0); + } + else { + CHECK(type->tp_version_tag == 0); + } + if (type->tp_flags & Py_TPFLAGS_DISALLOW_INSTANTIATION) { CHECK(type->tp_new == NULL); CHECK(PyDict_Contains(type->tp_dict, &_Py_ID(__new__)) == 0); @@ -4469,8 +4476,6 @@ _PyStaticType_Dealloc(PyTypeObject *type) } type->tp_flags &= ~Py_TPFLAGS_READY; - type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; - type->tp_version_tag = 0; if (type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) { _PyStaticType_ClearWeakRefs(type); @@ -6968,6 +6973,8 @@ PyType_Ready(PyTypeObject *type) /* Historically, all static types were immutable. See bpo-43908 */ if (!(type->tp_flags & Py_TPFLAGS_HEAPTYPE)) { + type->tp_version_tag = next_version_tag++; + type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; type->tp_flags |= Py_TPFLAGS_IMMUTABLETYPE; }
@erlend-aasland, your patch seems correct. I was going to ask what's the goal of your fix, since the merged fix addressed the originally described failure and gh-95795 addresses the rest. However, I think your fix might also deal with non-builtin static types (e.g. from community extension modules), which would probably be worth fixing too (regardless of how remote the possible failure).
I have some additional feedback, but I can wait until you've made a PR. (Feel free to request a review from me.)
Reacted by Erlend E. AaslandThanks for the comment, @ericsnowcurrently. I've alredy made a PR; I've requested your review. The PR is slightly different from my proposed patch here. I would also like to add some additional asserts, for example in
PyType_Modifiedandassign_version_tag(naming might differ; I'm on my phone right now.)As for the goal, I think Mark's reasoning is correct; making sure all immutable types have a lifelong (and immutable) valid version feels more correct than a fixup during finalise.
As for the goal, I think Mark's reasoning is correct; making sure all immutable types have a lifelong (and immutable) valid version feels more correct than a fixup during finalise.
That's the ideal goal yes but your PR only achieve valid version for only one cycle 1 of interpreter initialization, it is reset as
PyType_Readyis called on every cycle so not quite same as valid across the whole process lifetime. One side-effect is that it will now reset the tag for extension modules too which I avoided in my PR but I don't see any issues with doing that either.#95795 is the complete solution for this, there is a lot of info about this in that thread.
Footnotes
-
This is same as status quo with https://github.com/python/cpython/pull/101697 ↩
Reacted by Eric Snow and Erlend E. Aasland-
- added a commit that references this issue
on Feb 9, 2023 True, Kumar, my PR is not a complete solution.
However, I think your fix might also deal with non-builtin static types (e.g. from community extension modules), which would probably be worth fixing too (regardless of how remote the possible failure).
Actually #101742 does not does this. The reason is that once a type is initialized via
PyType_Readyit is never deallocated (in the sense that thetp_dictand other things are never cleared) and if the interpreter is reinitialized then it uses the same old initialized type.PyType_Readyreturns early if type is already initialized by previous interpreter.Lines 6957 to 6963 in d40a23c
So because of this #101742 has no effect because that code is only called once and subsequent initializations are skipped entirely. This happens in core static types because they are deallocated and reinitialized on every cycle unlike extension modules.
Actually #101742 does not does this.
Good point.
Based on the discussion, I suggest closing #101742 and instead go for a complete fix, as proposed in Kumar's issue #95795. I could give it a go, just for fun1. OTOH, I don't want to waste review time, so I'm fine with leaving it to Kumar.
Footnotes
-
it'd be a nice opportunity for me to getting to know other parts of the code base better. ↩
Reacted by Eric Snow and Kumar Aditya-
- added a commit that references this issue
on Feb 11, 2023 The backport is merged so closing.
I could give it a go, just for fun
@erlend-aasland I'd say give it a shot, the points of implementation are written here #95795 (comment) and we can work together on this.
Reacted by Erlend E. AaslandThanks, Kumar. I'll see if I can have something for you by the next weekend.
_PyStaticType_Deallocdoes not invalidate the type version tag and when the interpreter is reinitialized it still points to the old index. It should be cleared after the runtime finalization to avoid a rare case where there is a cache hit from the old index.Found while working on https://github.com/python/cpython/actions/runs/4112993949/jobs/7098592004
Linked PRs
_PyStaticType_Dealloc#101697_PyStaticType_Dealloc(GH-101697) #101722