[pypy-commit] pypy default: merge cpyext-obj-stealing which improves PyListObject refcounting compatibility
mattip
pypy.commits at gmail.com
Sat May 20 15:41:59 EDT 2017
Author: Matti Picus <matti.picus at gmail.com>
Branch:
Changeset: r91349:158d97f6e1eb
Date: 2017-05-20 22:31 +0300
http://bitbucket.org/pypy/pypy/changeset/158d97f6e1eb/
Log: merge cpyext-obj-stealing which improves PyListObject refcounting
compatibility
diff --git a/pypy/module/cpyext/api.py b/pypy/module/cpyext/api.py
--- a/pypy/module/cpyext/api.py
+++ b/pypy/module/cpyext/api.py
@@ -1557,7 +1557,6 @@
@specialize.memo()
def make_generic_cpy_call(FT, expect_null):
- from pypy.module.cpyext.pyobject import make_ref, from_ref
from pypy.module.cpyext.pyobject import is_pyobj, as_pyobj
from pypy.module.cpyext.pyobject import get_w_obj_and_decref
from pypy.module.cpyext.pyerrors import PyErr_Occurred
diff --git a/pypy/module/cpyext/include/listobject.h b/pypy/module/cpyext/include/listobject.h
--- a/pypy/module/cpyext/include/listobject.h
+++ b/pypy/module/cpyext/include/listobject.h
@@ -1,1 +1,1 @@
-#define PyList_GET_ITEM(o, i) PyList_GetItem((PyObject*)(o), (i))
+/* empty */
diff --git a/pypy/module/cpyext/listobject.py b/pypy/module/cpyext/listobject.py
--- a/pypy/module/cpyext/listobject.py
+++ b/pypy/module/cpyext/listobject.py
@@ -1,9 +1,10 @@
+from rpython.rlib.objectmodel import always_inline
from rpython.rtyper.lltypesystem import rffi, lltype
from pypy.module.cpyext.api import (cpython_api, CANNOT_FAIL, Py_ssize_t,
build_type_checkers)
from pypy.module.cpyext.pyerrors import PyErr_BadInternalCall
-from pypy.module.cpyext.pyobject import Py_DecRef, PyObject, make_ref
+from pypy.module.cpyext.pyobject import decref, incref, PyObject, make_ref
from pypy.objspace.std.listobject import W_ListObject
from pypy.interpreter.error import oefmt
@@ -19,59 +20,68 @@
PySequence_SetItem() or expose the object to Python code before
setting all items to a real object with PyList_SetItem().
"""
- return space.newlist([None] * len)
+ w_list = space.newlist([None] * len)
+ #w_list.convert_to_cpy_strategy(space)
+ return w_list
- at cpython_api([rffi.VOIDP, Py_ssize_t, PyObject], PyObject, error=CANNOT_FAIL,
- result_borrowed=True)
-def PyList_SET_ITEM(space, w_list, index, w_item):
- """Macro form of PyList_SetItem() without error checking. This is normally
+ at always_inline
+def get_list_storage(space, w_list):
+ from pypy.module.cpyext.sequence import CPyListStrategy
+ assert isinstance(w_list, W_ListObject)
+ w_list.convert_to_cpy_strategy(space)
+ return CPyListStrategy.unerase(w_list.lstorage)
+
+ at cpython_api([rffi.VOIDP, Py_ssize_t, PyObject], lltype.Void, error=CANNOT_FAIL)
+def PyList_SET_ITEM(space, w_list, index, py_item):
+ """Form of PyList_SetItem() without error checking. This is normally
only used to fill in new lists where there is no previous content.
- This function "steals" a reference to item, and, unlike PyList_SetItem(),
- does not discard a reference to any item that it being replaced; any
- reference in list at position i will be leaked.
+ "steals" a reference to item, and, unlike PyList_SetItem(), does not
+ discard a reference to any item that it being replaced; any reference in
+ list at position i will be leaked.
"""
- assert isinstance(w_list, W_ListObject)
+ storage = get_list_storage(space, w_list)
assert 0 <= index < w_list.length()
- # Deliberately leak, so that it can be safely decref'd.
- make_ref(space, w_list.getitem(index))
- Py_DecRef(space, w_item)
- w_list.setitem(index, w_item)
- return w_item
-
+ storage._elems[index] = py_item
@cpython_api([PyObject, Py_ssize_t, PyObject], rffi.INT_real, error=-1)
-def PyList_SetItem(space, w_list, index, w_item):
+def PyList_SetItem(space, w_list, index, py_item):
"""Set the item at index index in list to item. Return 0 on success
or -1 on failure.
-
+
This function "steals" a reference to item and discards a reference to
an item already in the list at the affected position.
"""
- Py_DecRef(space, w_item)
if not isinstance(w_list, W_ListObject):
+ decref(space, w_item)
PyErr_BadInternalCall(space)
if index < 0 or index >= w_list.length():
+ decref(space, w_item)
raise oefmt(space.w_IndexError, "list assignment index out of range")
- w_list.setitem(index, w_item)
+ storage = get_list_storage(space, w_list)
+ py_old = storage._elems[index]
+ storage._elems[index] = py_item
+ decref(w_list.space, py_old)
return 0
- at cpython_api([PyObject, Py_ssize_t], PyObject, result_borrowed=True)
+ at cpython_api([rffi.VOIDP, Py_ssize_t], PyObject, result_is_ll=True)
+def PyList_GET_ITEM(space, w_list, index):
+ storage = get_list_storage(space, w_list)
+ assert 0 <= index < w_list.length()
+ return storage._elems[index] # borrowed ref
+
+ at cpython_api([PyObject, Py_ssize_t], PyObject, result_is_ll=True)
def PyList_GetItem(space, w_list, index):
"""Return the object at position pos in the list pointed to by p. The
position must be positive, indexing from the end of the list is not
supported. If pos is out of bounds, return NULL and set an
IndexError exception."""
- from pypy.module.cpyext.sequence import CPyListStrategy
if not isinstance(w_list, W_ListObject):
PyErr_BadInternalCall(space)
if index < 0 or index >= w_list.length():
raise oefmt(space.w_IndexError, "list index out of range")
- cpy_strategy = space.fromcache(CPyListStrategy)
- if w_list.strategy is not cpy_strategy:
- w_list.ensure_object_strategy() # make sure we can return a borrowed obj
- w_res = w_list.getitem(index)
- return w_res # borrowed ref
+ storage = get_list_storage(space, w_list)
+ return storage._elems[index] # borrowed ref
@cpython_api([PyObject, PyObject], rffi.INT_real, error=-1)
diff --git a/pypy/module/cpyext/pyobject.py b/pypy/module/cpyext/pyobject.py
--- a/pypy/module/cpyext/pyobject.py
+++ b/pypy/module/cpyext/pyobject.py
@@ -30,8 +30,7 @@
return subtype_dealloc.api_func
def allocate(self, space, w_type, itemcount=0):
- # similar to PyType_GenericAlloc?
- # except that it's not related to any pypy object.
+ # typically called from PyType_GenericAlloc via typedescr.allocate
# this returns a PyObject with ob_refcnt == 1.
pytype = as_pyobj(space, w_type)
@@ -250,6 +249,8 @@
w_obj = rawrefcount.to_obj(W_Root, pyobj)
return w_obj is not None and w_obj is not w_marker_deallocating
+def w_obj_has_pyobj(w_obj):
+ return bool(rawrefcount.from_obj(PyObject, w_obj))
def is_pyobj(x):
if x is None or isinstance(x, W_Root):
@@ -275,13 +276,14 @@
"""
if is_pyobj(obj):
pyobj = rffi.cast(PyObject, obj)
+ at_least = 1
else:
pyobj = as_pyobj(space, obj, w_userdata)
+ at_least = rawrefcount.REFCNT_FROM_PYPY
if pyobj:
- assert pyobj.c_ob_refcnt > 0
+ assert pyobj.c_ob_refcnt >= at_least
pyobj.c_ob_refcnt += 1
- if not is_pyobj(obj):
- keepalive_until_here(obj)
+ keepalive_until_here(obj)
return pyobj
@@ -315,9 +317,14 @@
obj = rffi.cast(PyObject, obj)
if obj:
assert obj.c_ob_refcnt > 0
+ assert obj.c_ob_pypy_link == 0 or obj.c_ob_refcnt > rawrefcount.REFCNT_FROM_PYPY
obj.c_ob_refcnt -= 1
if obj.c_ob_refcnt == 0:
_Py_Dealloc(space, obj)
+ #else:
+ # w_obj = rawrefcount.to_obj(W_Root, ref)
+ # if w_obj is not None:
+ # assert obj.c_ob_refcnt >= rawrefcount.REFCNT_FROM_PYPY
else:
get_w_obj_and_decref(space, obj)
diff --git a/pypy/module/cpyext/sequence.py b/pypy/module/cpyext/sequence.py
--- a/pypy/module/cpyext/sequence.py
+++ b/pypy/module/cpyext/sequence.py
@@ -256,8 +256,9 @@
def setitem(self, w_list, index, w_obj):
storage = self.unerase(w_list.lstorage)
index = self._check_index(index, storage._length)
- decref(w_list.space, storage._elems[index])
+ py_old = storage._elems[index]
storage._elems[index] = make_ref(w_list.space, w_obj)
+ decref(w_list.space, py_old)
def length(self, w_list):
storage = self.unerase(w_list.lstorage)
diff --git a/pypy/module/cpyext/test/array.c b/pypy/module/cpyext/test/array.c
--- a/pypy/module/cpyext/test/array.c
+++ b/pypy/module/cpyext/test/array.c
@@ -1872,6 +1872,7 @@
for (nn = 0; nn < n; nn++)
{
v = PyList_GetItem(obj1, nn);
+ Py_INCREF(v);
PyList_SetItem(ret, nn+ii*n, v);
}
return ret;
@@ -1887,6 +1888,7 @@
for (nn = 0; nn < n; nn++)
{
v = PyList_GetItem(obj2, nn);
+ Py_INCREF(v);
PyList_SetItem(ret, nn+ii*n, v);
}
return ret;
diff --git a/pypy/module/cpyext/test/test_listobject.py b/pypy/module/cpyext/test/test_listobject.py
--- a/pypy/module/cpyext/test/test_listobject.py
+++ b/pypy/module/cpyext/test/test_listobject.py
@@ -38,11 +38,13 @@
assert api.PyList_Insert(w_l, 0, space.wrap(1)) == 0
assert api.PyList_Size(w_l) == 3
assert api.PyList_Insert(w_l, 99, space.wrap(2)) == 0
- assert space.unwrap(api.PyList_GetItem(w_l, 3)) == 2
+ assert api.PyObject_Compare(api.PyList_GetItem(w_l, 3),
+ space.wrap(2)) == 0
# insert at index -1: next-to-last
assert api.PyList_Insert(w_l, -1, space.wrap(3)) == 0
- assert space.unwrap(api.PyList_GetItem(w_l, 3)) == 3
-
+ assert api.PyObject_Compare(api.PyList_GET_ITEM(w_l, 3),
+ space.wrap(3)) == 0
+
def test_sort(self, space, api):
l = space.newlist([space.wrap(1), space.wrap(0), space.wrap(7000)])
assert api.PyList_Sort(l) == 0
@@ -152,29 +154,35 @@
def test_list_macros(self):
"""The PyList_* macros cast, and calls expecting that build."""
module = self.import_extension('foo', [
- ("test_macro_invocations", "METH_NOARGS",
+ ("test_macro_invocations", "METH_O",
"""
PyObject* o = PyList_New(2);
PyListObject* l = (PyListObject*)o;
+ Py_INCREF(args);
+ Py_INCREF(args);
+ PyList_SET_ITEM(o, 0, args);
+ PyList_SET_ITEM(l, 1, args);
- Py_INCREF(o);
- PyList_SET_ITEM(o, 0, o);
- Py_INCREF(o);
- PyList_SET_ITEM(l, 1, o);
+ if(PyList_GET_ITEM(o, 0) != PyList_GET_ITEM(l, 1))
+ {
+ PyErr_SetString(PyExc_AssertionError, "PyList_GET_ITEM error");
+ return NULL;
+ }
- PyList_GET_ITEM(o, 0);
- PyList_GET_ITEM(l, 1);
-
- PyList_GET_SIZE(o);
- PyList_GET_SIZE(l);
+ if(PyList_GET_SIZE(o) != PyList_GET_SIZE(l))
+ {
+ PyErr_SetString(PyExc_AssertionError, "PyList_GET_SIZE error");
+ return NULL;
+ }
return o;
"""
)
])
- x = module.test_macro_invocations()
- assert x[0] is x[1] is x
+ s = 'abcdef'
+ x = module.test_macro_invocations(s)
+ assert x[0] is x[1] is s
def test_get_item_macro(self):
module = self.import_extension('foo', [
@@ -183,39 +191,96 @@
PyObject* o, *o2, *o3;
o = PyList_New(1);
- o2 = PyInt_FromLong(0);
+ o2 = PyBytes_FromString("test_get_item0");
+ Py_INCREF(o2);
PyList_SET_ITEM(o, 0, o2);
- o2 = NULL;
o3 = PyList_GET_ITEM(o, 0);
Py_INCREF(o3);
- Py_CLEAR(o);
+ Py_DECREF(o);
+ Py_DECREF(o2);
return o3;
""")])
- assert module.test_get_item() == 0
+ assert module.test_get_item() == b'test_get_item0'
- def test_set_item_macro(self):
+ def test_item_refcounts(self):
"""PyList_SET_ITEM leaks a reference to the target."""
module = self.import_extension('foo', [
- ("test_refcount_diff_after_setitem", "METH_NOARGS",
+ ("test_refcount_diff", "METH_VARARGS",
"""
- PyObject* o = PyList_New(0);
- PyObject* o2 = PyList_New(0);
- Py_ssize_t refcount, new_refcount;
+ /* test that the refcount differences for functions
+ * are correct. diff1 - expected refcount diff for i1,
+ * diff2 - expected refcount diff for i2
+ */
+ #define CHECKCOUNT(diff1, diff2, action) \
+ new_count1 = Py_REFCNT(i1); \
+ new_count2 = Py_REFCNT(i2); \
+ diff = new_count1 - old_count1; \
+ if (diff != diff1) {\
+ sprintf(errbuffer, action \
+ " i1 expected diff of %ld got %ld", (long)diff1, (long)diff); \
+ PyErr_SetString(PyExc_AssertionError, errbuffer); \
+ return NULL; } \
+ diff = new_count2 - old_count2; \
+ if (diff != diff2) {\
+ sprintf(errbuffer, action \
+ " i2 expected diff of %ld got %ld", (long)diff2, (long)diff); \
+ PyErr_SetString(PyExc_AssertionError, errbuffer); \
+ return NULL; } \
+ old_count1 = new_count1; \
+ old_count2 = new_count2;
- PyList_Append(o, o2); // does not steal o2
+ PyObject* tmp, *o = PyList_New(0);
+ char errbuffer[1024];
+ PyObject* i1 = PyTuple_GetItem(args, 0);
+ PyObject* i2 = PyTuple_GetItem(args, 1);
+ Py_ssize_t old_count1, new_count1;
+ Py_ssize_t old_count2, new_count2;
+ Py_ssize_t diff;
+ int ret;
- refcount = Py_REFCNT(o2);
+ old_count1 = Py_REFCNT(i1);
+ old_count2 = Py_REFCNT(i2);
- // Steal a reference to o2, but leak the old reference to o2.
- // The net result should be no change in refcount.
- PyList_SET_ITEM(o, 0, o2);
+ ret = PyList_Append(o, i1);
+ if (ret != 0)
+ return NULL;
+ /* check the result of Append(), and also force the list
+ to use the CPyListStrategy now */
+ if (PyList_GET_ITEM(o, 0) != i1)
+ {
+ PyErr_SetString(PyExc_AssertionError, "Append() error?");
+ return NULL;
+ }
+ CHECKCOUNT(1, 0, "PyList_Append");
- new_refcount = Py_REFCNT(o2);
+ Py_INCREF(i2); /* for PyList_SET_ITEM */
+ CHECKCOUNT(0, 1, "Py_INCREF");
- Py_CLEAR(o);
- Py_DECREF(o2); // append incref'd.
- // Py_CLEAR(o2); // naive implementation would fail here.
- return PyLong_FromSsize_t(new_refcount - refcount);
+ PyList_SET_ITEM(o, 0, i2);
+ CHECKCOUNT(0, 0, "PyList_SET_ITEM");
+
+ tmp = PyList_GET_ITEM(o, 0);
+ if (tmp != i2)
+ {
+ PyErr_SetString(PyExc_AssertionError, "SetItem() error?");
+ return NULL;
+ }
+ CHECKCOUNT(0, 0, "PyList_GET_ITEM");
+
+ PyList_SetItem(o, 0, i1);
+ CHECKCOUNT(0, -1, "PyList_Set_Item");
+
+ PyList_GetItem(o, 0);
+ CHECKCOUNT(0, 0, "PyList_Get_Item");
+
+ Py_DECREF(o);
+ #ifndef PYPY_VERSION
+ {
+ // PyPy deletes only at teardown
+ CHECKCOUNT(-1, 0, "Py_DECREF(o)");
+ }
+ #endif
+ return PyLong_FromSsize_t(0);
""")])
- assert module.test_refcount_diff_after_setitem() == 0
+ assert module.test_refcount_diff(["first"], ["second"]) == 0
diff --git a/pypy/module/cpyext/typeobject.py b/pypy/module/cpyext/typeobject.py
--- a/pypy/module/cpyext/typeobject.py
+++ b/pypy/module/cpyext/typeobject.py
@@ -24,7 +24,7 @@
W_PyCMethodObject, W_PyCFunctionObject)
from pypy.module.cpyext.modsupport import convert_method_defs
from pypy.module.cpyext.pyobject import (
- PyObject, make_ref, create_ref, from_ref, get_typedescr, make_typedescr,
+ PyObject, make_ref, from_ref, get_typedescr, make_typedescr,
track_reference, Py_DecRef, as_pyobj)
from pypy.module.cpyext.slotdefs import (
slotdefs_for_tp_slots, slotdefs_for_wrappers, get_slot_tp_function,
diff --git a/pypy/objspace/std/listobject.py b/pypy/objspace/std/listobject.py
--- a/pypy/objspace/std/listobject.py
+++ b/pypy/objspace/std/listobject.py
@@ -230,15 +230,13 @@
return list(items)
def switch_to_object_strategy(self):
+ object_strategy = self.space.fromcache(ObjectListStrategy)
+ if self.strategy is object_strategy:
+ return
list_w = self.getitems()
- object_strategy = self.space.fromcache(ObjectListStrategy)
self.strategy = object_strategy
object_strategy.init_from_list_w(self, list_w)
- def ensure_object_strategy(self): # for cpyext
- if self.strategy is not self.space.fromcache(ObjectListStrategy):
- self.switch_to_object_strategy()
-
def _temporarily_as_objects(self):
if self.strategy is self.space.fromcache(ObjectListStrategy):
return self
More information about the pypy-commit
mailing list