added 3 commits
July 14, 2022 13:37jcrist added a commit that referenced this pull request
Jul 21, 2022This 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.
Merged
jcrist added a commit that referenced this pull request
Jul 21, 2022This 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, 2022This 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.