Python 3.11 support & testing - #146
Merged
Merged
Conversation
This fixes issues caused by Python 3.11 changing some internal APIs (mostly around GC implementation).
Contributor
|
😲 |
jcrist
added a commit
that referenced
this pull request
Jul 21, 2022
This fixes a segfault introduced in #146. This bug was never released. - Before Python 3.11, calling `PyObject_GC_Del` worked the same as `PyObject_Del`, where it was functionally equivalent to `free`, only treating the argument as an opaque pointer. - In Python 3.11, `PyObject_GC_Del` accesses the `ob_type` field to determine the size of the GC header. This didn't play well with our freelist implementation. - To work around this, #146 moved to keeping the `ob_type` field on objects in the freelist, only decref'ing their type when the memory is used, or when it is cleared by `Struct_freelist_clear`. - However, `Struct_freelist_clear` is called during a GC traversal. In rare circumstances, an object stored in our freelist can contain the only reference to a `StructMeta` type object. Decref'ing `ob_type` here would lead to that `StructMeta` object being freed *during the GC traversal*. Freeing GC types during a GC traversal can do weird things with the GC linked list, leading to segfaults. This bug is rare, since in most cases Struct types are defined at the top level of a module, and live for the duration of the program. It also requires the GC to be called at the proper time and have the freelist in the proper order to trigger the bug. All of this is complicated. The fix here is to: - Always decref the type immediately in `Struct_dealloc` - Replace the `ob_type` fields for struct memory blocks stored in our freelist with references to known non-heap types. `PyObject_GC_Del` only checks `tp_flags` on the type, so the actual type doesn't really matter. We use a non-heap type so that we know the type reference will live for the duration of the program, and no incref/decref behavior will be needed. This was a complicated bug to track down though, in the future we should re-evaluate whether the freelist is worth it for actual programs. It gives a measurable boost for microbenchmarks (which many users will use to judge performance), but I'm not positive it really matters for real world use.
jcrist
added a commit
that referenced
this pull request
Jul 21, 2022
This fixes a segfault introduced in #146. This bug was never released. - Before Python 3.11, calling `PyObject_GC_Del` worked the same as `PyObject_Del`, where it was functionally equivalent to `free`, only treating the argument as an opaque pointer. - In Python 3.11, `PyObject_GC_Del` accesses the `ob_type` field to determine the size of the GC header. This didn't play well with our freelist implementation. - To work around this, #146 moved to keeping the `ob_type` field on objects in the freelist, only decref'ing their type when the memory is used, or when it is cleared by `Struct_freelist_clear`. - However, `Struct_freelist_clear` is called during a GC traversal. In rare circumstances, an object stored in our freelist can contain the only reference to a `StructMeta` type object. Decref'ing `ob_type` here would lead to that `StructMeta` object being freed *during the GC traversal*. Freeing GC types during a GC traversal can do weird things with the GC linked list, leading to segfaults. This bug is rare, since in most cases Struct types are defined at the top level of a module, and live for the duration of the program. It also requires the GC to be called at the proper time and have the freelist in the proper order to trigger the bug. All of this is complicated. The fix here is to: - Always decref the type immediately in `Struct_dealloc` - Replace the `ob_type` fields for struct memory blocks stored in our freelist with references to known non-heap types. `PyObject_GC_Del` only checks `tp_flags` on the type, so the actual type doesn't really matter. We use a non-heap type so that we know the type reference will live for the duration of the program, and no incref/decref behavior will be needed. This was a complicated bug to track down though, in the future we should re-evaluate whether the freelist is worth it for actual programs. It gives a measurable boost for microbenchmarks (which many users will use to judge performance), but I'm not positive it really matters for real world use.
jcrist
added a commit
that referenced
this pull request
Jul 21, 2022
This fixes a segfault introduced in #146. This bug was never released. - Before Python 3.11, calling `PyObject_GC_Del` worked the same as `PyObject_Del`, where it was functionally equivalent to `free`, only treating the argument as an opaque pointer. - In Python 3.11, `PyObject_GC_Del` accesses the `ob_type` field to determine the size of the GC header. This didn't play well with our freelist implementation. - To work around this, #146 moved to keeping the `ob_type` field on objects in the freelist, only decref'ing their type when the memory is used, or when it is cleared by `Struct_freelist_clear`. - However, `Struct_freelist_clear` is called during a GC traversal. In rare circumstances, an object stored in our freelist can contain the only reference to a `StructMeta` type object. Decref'ing `ob_type` here would lead to that `StructMeta` object being freed *during the GC traversal*. Freeing GC types during a GC traversal can do weird things with the GC linked list, leading to segfaults. This bug is rare, since in most cases Struct types are defined at the top level of a module, and live for the duration of the program. It also requires the GC to be called at the proper time and have the freelist in the proper order to trigger the bug. All of this is complicated. The fix here is to: - Always decref the type immediately in `Struct_dealloc` - Replace the `ob_type` fields for struct memory blocks stored in our freelist with references to known non-heap types. `PyObject_GC_Del` only checks `tp_flags` on the type, so the actual type doesn't really matter. We use a non-heap type so that we know the type reference will live for the duration of the program, and no incref/decref behavior will be needed. This was a complicated bug to track down though, in the future we should re-evaluate whether the freelist is worth it for actual programs. It gives a measurable boost for microbenchmarks (which many users will use to judge performance), but I'm not positive it really matters for real world use.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This:
Fixes #143