Merge pull request #2842 from jdemeyer/trashcan

@cython.trashcan directive to enable the Python trashcan for deallocations

Cython/Compiler/ModuleNode.py

@@ -1426,6 +1426,7 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
 
         is_final_type = scope.parent_type.is_final_type
         needs_gc = scope.needs_gc()
+        needs_trashcan = scope.needs_trashcan()
 
         weakref_slot = scope.lookup_here("__weakref__") if not scope.is_closure_class_scope else None
         if weakref_slot not in scope.var_entries:
@@ -1464,6 +1465,11 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
             # running this destructor.
             code.putln("PyObject_GC_UnTrack(o);")
 
+        if needs_trashcan:
+            code.globalstate.use_utility_code(
+                UtilityCode.load_cached("PyTrashcan", "ExtensionTypes.c"))
+            code.putln("__Pyx_TRASHCAN_BEGIN(o, %s)" % slot_func_cname)
+
         # call the user's __dealloc__
         self.generate_usr_dealloc_call(scope, code)
 
@@ -1537,6 +1543,10 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
             code.putln("(*Py_TYPE(o)->tp_free)(o);")
             if freelist_size:
                 code.putln("}")
+
+        if needs_trashcan:
+            code.putln("__Pyx_TRASHCAN_END")
+
         code.putln(
             "}")
 

Cython/Compiler/Options.py

@@ -317,6 +317,7 @@ directive_types = {
     'freelist': int,
     'c_string_type': one_of('bytes', 'bytearray', 'str', 'unicode'),
     'c_string_encoding': normalise_encoding_name,
+    'trashcan': bool,
 }
 
 for key, val in _directive_defaults.items():
@@ -359,6 +360,7 @@ directive_scopes = {  # defaults to available everywhere
     'np_pythran': ('module',),
     'fast_gil': ('module',),
     'iterable_coroutine': ('module', 'function'),
+    'trashcan' : ('cclass',),
 }
 
 

Cython/Compiler/PyrexTypes.py

@@ -1136,6 +1136,7 @@ class PyObjectType(PyrexType):
     is_extern = False
     is_subclassed = False
     is_gc_simple = False
+    builtin_trashcan = False  # builtin type using trashcan
 
     def __str__(self):
         return "Python object"
@@ -1190,10 +1191,14 @@ class PyObjectType(PyrexType):
 
 
 builtin_types_that_cannot_create_refcycles = set([
-    'bool', 'int', 'long', 'float', 'complex',
+    'object', 'bool', 'int', 'long', 'float', 'complex',
     'bytearray', 'bytes', 'unicode', 'str', 'basestring'
 ])
 
+builtin_types_with_trashcan = set([
+    'dict', 'list', 'set', 'frozenset', 'tuple', 'type',
+])
+
 
 class BuiltinObjectType(PyObjectType):
     #  objstruct_cname  string           Name of PyObject struct
@@ -1218,6 +1223,7 @@ class BuiltinObjectType(PyObjectType):
         self.typeptr_cname = "(&%s)" % cname
         self.objstruct_cname = objstruct_cname
         self.is_gc_simple = name in builtin_types_that_cannot_create_refcycles
+        self.builtin_trashcan = name in builtin_types_with_trashcan
         if name == 'type':
             # Special case the type type, as many C API calls (and other
             # libraries) actually expect a PyTypeObject* for type arguments.

Cython/Compiler/Symtab.py

@@ -2041,7 +2041,7 @@ class PyClassScope(ClassScope):
 class CClassScope(ClassScope):
     #  Namespace of an extension type.
     #
-    #  parent_type           CClassType
+    #  parent_type           PyExtensionType
     #  #typeobj_cname        string or None
     #  #objstruct_cname      string
     #  method_table_cname    string
@@ -2085,6 +2085,22 @@ class CClassScope(ClassScope):
             return not self.parent_type.is_gc_simple
         return False
 
+    def needs_trashcan(self):
+        # If the trashcan directive is explicitly set to False,
+        # unconditionally disable the trashcan.
+        directive = self.directives.get('trashcan')
+        if directive is False:
+            return False
+        # If the directive is set to True and the class has Python-valued
+        # C attributes, then it should use the trashcan in tp_dealloc.
+        if directive and self.has_cyclic_pyobject_attrs:
+            return True
+        # Use the trashcan if the base class uses it
+        base_type = self.parent_type.base_type
+        if base_type and base_type.scope is not None:
+            return base_type.scope.needs_trashcan()
+        return self.parent_type.builtin_trashcan
+
     def needs_tp_clear(self):
         """
         Do we need to generate an implementation for the tp_clear slot? Can

Cython/Utility/ExtensionTypes.c

@@ -74,6 +74,49 @@ static int __Pyx_PyType_Ready(PyTypeObject *t) {
     return r;
 }
 
+/////////////// PyTrashcan.proto ///////////////
+
+// These macros are taken from https://github.com/python/cpython/pull/11841
+// Unlike the Py_TRASHCAN_SAFE_BEGIN/Py_TRASHCAN_SAFE_END macros, they
+// allow dealing correctly with subclasses.
+
+// This requires CPython version >= 2.7.4
+// (or >= 3.2.4 but we don't support such old Python 3 versions anyway)
+#if CYTHON_COMPILING_IN_CPYTHON && PY_VERSION_HEX >= 0x02070400
+#define __Pyx_TRASHCAN_BEGIN_CONDITION(op, cond) \
+    do { \
+        PyThreadState *_tstate = NULL; \
+        // If "cond" is false, then _tstate remains NULL and the deallocator
+        // is run normally without involving the trashcan
+        if (cond) { \
+            _tstate = PyThreadState_GET(); \
+            if (_tstate->trash_delete_nesting >= PyTrash_UNWIND_LEVEL) { \
+                // Store the object (to be deallocated later) and jump past
+                // Py_TRASHCAN_END, skipping the body of the deallocator
+                _PyTrash_thread_deposit_object((PyObject*)(op)); \
+                break; \
+            } \
+            ++_tstate->trash_delete_nesting; \
+        }
+        // The body of the deallocator is here.
+#define __Pyx_TRASHCAN_END \
+        if (_tstate) { \
+            --_tstate->trash_delete_nesting; \
+            if (_tstate->trash_delete_later && _tstate->trash_delete_nesting <= 0) \
+                _PyTrash_thread_destroy_chain(); \
+        } \
+    } while (0);
+
+#define __Pyx_TRASHCAN_BEGIN(op, dealloc) __Pyx_TRASHCAN_BEGIN_CONDITION(op, \
+        Py_TYPE(op)->tp_dealloc == (destructor)(dealloc))
+
+#else
+// The trashcan is a no-op on other Python implementations
+// or old CPython versions
+#define __Pyx_TRASHCAN_BEGIN(op, dealloc)
+#define __Pyx_TRASHCAN_END
+#endif
+
 /////////////// CallNextTpDealloc.proto ///////////////
 
 static void __Pyx_call_next_tp_dealloc(PyObject* obj, destructor current_tp_dealloc);

tests/run/trashcan.pyx

+# mode: run
+
+cimport cython
+
+
+# Count number of times an object was deallocated twice. This should remain 0.
+cdef int double_deallocations = 0
+def assert_no_double_deallocations():
+    global double_deallocations
+    err = double_deallocations
+    double_deallocations = 0
+    assert not err
+
+
+# Compute x = f(f(f(...(None)...))) nested n times and throw away the result.
+# The real test happens when exiting this function: then a big recursive
+# deallocation of x happens. We are testing two things in the tests below:
+# that Python does not crash and that no double deallocation happens.
+# See also https://github.com/python/cpython/pull/11841
+def recursion_test(f, int n=2**20):
+    x = None
+    cdef int i
+    for i in range(n):
+        x = f(x)
+
+
+@cython.trashcan(True)
+cdef class Recurse:
+    """
+    >>> recursion_test(Recurse)
+    >>> assert_no_double_deallocations()
+    """
+    cdef public attr
+    cdef int deallocated
+
+    def __init__(self, x):
+        self.attr = x
+
+    def __dealloc__(self):
+        # Check that we're not being deallocated twice
+        global double_deallocations
+        double_deallocations += self.deallocated
+        self.deallocated = 1
+
+
+cdef class RecurseSub(Recurse):
+    """
+    >>> recursion_test(RecurseSub)
+    >>> assert_no_double_deallocations()
+    """
+    cdef int subdeallocated
+
+    def __dealloc__(self):
+        # Check that we're not being deallocated twice
+        global double_deallocations
+        double_deallocations += self.subdeallocated
+        self.subdeallocated = 1
+
+
+@cython.freelist(4)
+@cython.trashcan(True)
+cdef class RecurseFreelist:
+    """
+    >>> recursion_test(RecurseFreelist)
+    >>> recursion_test(RecurseFreelist, 1000)
+    >>> assert_no_double_deallocations()
+    """
+    cdef public attr
+    cdef int deallocated
+
+    def __init__(self, x):
+        self.attr = x
+
+    def __dealloc__(self):
+        # Check that we're not being deallocated twice
+        global double_deallocations
+        double_deallocations += self.deallocated
+        self.deallocated = 1
+
+
+# Subclass of list => uses trashcan by default
+# As long as https://github.com/python/cpython/pull/11841 is not fixed,
+# this does lead to double deallocations, so we skip that check.
+cdef class RecurseList(list):
+    """
+    >>> RecurseList(42)
+    [42]
+    >>> recursion_test(RecurseList)
+    """
+    def __init__(self, x):
+        super().__init__((x,))
+
+
+# Some tests where the trashcan is NOT used. When the trashcan is not used
+# in a big recursive deallocation, the __dealloc__s of the base classs are
+# only run after the __dealloc__s of the subclasses.
+# We use this to detect trashcan usage.
+cdef int base_deallocated = 0
+cdef int trashcan_used = 0
+def assert_no_trashcan_used():
+    global base_deallocated, trashcan_used
+    err = trashcan_used
+    trashcan_used = base_deallocated = 0
+    assert not err
+
+
+cdef class Base:
+    def __dealloc__(self):
+        global base_deallocated
+        base_deallocated = 1
+
+
+# Trashcan disabled by default
+cdef class Sub1(Base):
+    """
+    >>> recursion_test(Sub1, 100)
+    >>> assert_no_trashcan_used()
+    """
+    cdef public attr
+
+    def __init__(self, x):
+        self.attr = x
+
+    def __dealloc__(self):
+        global base_deallocated, trashcan_used
+        trashcan_used += base_deallocated
+
+
+@cython.trashcan(True)
+cdef class Middle(Base):
+    cdef public foo
+
+
+# Trashcan disabled explicitly
+@cython.trashcan(False)
+cdef class Sub2(Middle):
+    """
+    >>> recursion_test(Sub2, 1000)
+    >>> assert_no_trashcan_used()
+    """
+    cdef public attr
+
+    def __init__(self, x):
+        self.attr = x
+
+    def __dealloc__(self):
+        global base_deallocated, trashcan_used
+        trashcan_used += base_deallocated