Skip to content

Commit 07c75e2

Browse files
msimacekmsimacek
committed
Send storage to native on stealing set calls
Our current implementation of reference stealing is incorrect and creating a correct one seems very complicated. Native storages already have the correct refcounting semantics, so just use them.
1 parent f9f6b5f commit 07c75e2

File tree

9 files changed

+87
-199
lines changed

9 files changed

+87
-199
lines changed

‎graalpython/com.oracle.graal.python.cext/include/cpython/listobject.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,12 @@ static inline Py_ssize_t PyList_GET_SIZE(PyObject *op) {
4545

4646
#define PyList_GET_ITEM(op, index) (PyList_GetItem((PyObject*)(op), (index)))
4747

48-
PyAPI_FUNC(void) PyTruffleList_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value);
48+
// GraalPy-specific
49+
PyAPI_FUNC(PyObject **) PyTruffleList_GetItems(PyObject *op);
50+
4951
static inline void
5052
PyList_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value) {
51-
PyTruffleList_SET_ITEM(op, index, value);
53+
PyTruffleList_GetItems(op)[index] = value;
5254
}
5355
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000
5456
#define PyList_SET_ITEM(op, index, value) \

‎graalpython/com.oracle.graal.python.cext/include/cpython/tupleobject.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,13 @@ static inline Py_ssize_t PyTuple_GET_SIZE(PyObject *op) {
3636
PyAPI_FUNC(PyObject *) _PyTuple_GET_ITEM(PyObject *, Py_ssize_t);
3737
#define PyTuple_GET_ITEM(op, i) _PyTuple_GET_ITEM(_PyObject_CAST(op), (i))
3838

39+
// GraalPy-specific
40+
PyAPI_FUNC(PyObject **) PyTruffleTuple_GetItems(PyObject *op);
41+
3942
/* Function *only* to be used to fill in brand new tuples */
40-
PyAPI_FUNC(void) PyTruffleTuple_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value);
4143
static inline void
4244
PyTuple_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value) {
43-
PyTruffleTuple_SET_ITEM(op, index, value);
45+
PyTruffleTuple_GetItems(op)[index] = value;
4446
}
4547
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000
4648
#define PyTuple_SET_ITEM(op, index, value) \

‎graalpython/com.oracle.graal.python.cext/src/abstract.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3062,7 +3062,11 @@ PyNumber_InPlaceOr(PyObject *v, PyObject *w)
30623062
PyObject **
30633063
PyTruffleSequence_Fast_ITEMS(PyObject *o)
30643064
{
3065-
return PyList_Check(o) ? PyListObject_ob_item(o) : PyTupleObject_ob_item(o);
3065+
if (PyTuple_Check(o)) {
3066+
return PyTruffleTuple_GetItems(o);
3067+
} else {
3068+
return PyTruffleList_GetItems(o);
3069+
}
30663070
}
30673071

30683072
PyObject*

‎graalpython/com.oracle.graal.python.cext/src/bytesobject.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ PyBytes_FromString(const char *str)
153153
PyObject *
154154
PyBytes_FromFormatV(const char *format, va_list vargs)
155155
{
156+
// GraalPy change: different implementation
156157
/* Unfortunately, we need to know the expected types of the arguments before we can do an upcall. */
157158
char *s;
158159
const char *f = format;
@@ -291,7 +292,7 @@ PyBytes_FromFormatV(const char *format, va_list vargs)
291292
entry = PyLong_FromVoidPtr(va_arg(vargs, void*));
292293
break;
293294
}
294-
GraalPyTuple_SetItem(args, i, entry);
295+
PyTuple_SET_ITEM(args, i, entry);
295296
}
296297
PyObject* result = GraalPyTruffleBytes_FromFormat(format, args);
297298
Py_DecRef(args);

‎graalpython/com.oracle.graal.python.cext/src/listobject.c

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,5 +42,47 @@
4242

4343
// alias for internal function, currently used in PyO3
4444
void _PyList_SET_ITEM(PyObject* a, Py_ssize_t b, PyObject* c) {
45-
return PyTruffleList_SET_ITEM(a, b, c);
45+
return PyList_SET_ITEM(a, b, c);
46+
}
47+
48+
static inline int
49+
valid_index(Py_ssize_t i, Py_ssize_t limit)
50+
{
51+
/* The cast to size_t lets us use just a single comparison
52+
to check whether i is in the range: 0 <= i < limit.
53+
54+
See: Section 14.2 "Bounds Checking" in the Agner Fog
55+
optimization manual found at:
56+
https://www.agner.org/optimize/optimizing_cpp.pdf
57+
*/
58+
return (size_t) i < (size_t) limit;
59+
}
60+
61+
int
62+
PyList_SetItem(PyObject *op, Py_ssize_t i,
63+
PyObject *newitem)
64+
{
65+
PyObject **p;
66+
if (!PyList_Check(op)) {
67+
Py_XDECREF(newitem);
68+
PyErr_BadInternalCall();
69+
return -1;
70+
}
71+
if (!valid_index(i, Py_SIZE(op))) {
72+
Py_XDECREF(newitem);
73+
PyErr_SetString(PyExc_IndexError,
74+
"list assignment index out of range");
75+
return -1;
76+
}
77+
// GraalPy change: avoid direct struct access
78+
p = PyTruffleList_GetItems(op) + i;
79+
Py_XSETREF(*p, newitem);
80+
return 0;
81+
}
82+
83+
// GraalPy-additions
84+
PyObject **
85+
PyTruffleList_GetItems(PyObject *op)
86+
{
87+
return PyListObject_ob_item(op);
4688
}

‎graalpython/com.oracle.graal.python.cext/src/tupleobject.c

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -136,12 +136,12 @@ PyTuple_GetItem(PyObject *op, Py_ssize_t i) {
136136
#endif /* GRAALVM_PYTHON_LLVM_MANAGED */
137137
}
138138

139-
#if 0 // GraalPy change
140139
int
141140
PyTuple_SetItem(PyObject *op, Py_ssize_t i, PyObject *newitem)
142141
{
143142
PyObject **p;
144-
if (!PyTuple_Check(op) || Py_REFCNT(op) != 1) {
143+
// GraalPy change: remove refcount check
144+
if (!PyTuple_Check(op)) {
145145
Py_XDECREF(newitem);
146146
PyErr_BadInternalCall();
147147
return -1;
@@ -152,11 +152,13 @@ PyTuple_SetItem(PyObject *op, Py_ssize_t i, PyObject *newitem)
152152
"tuple assignment index out of range");
153153
return -1;
154154
}
155-
p = ((PyTupleObject *)op) -> ob_item + i;
155+
// GraalPy change: avoid direct struct access
156+
p = PyTruffleTuple_GetItems(op) + i;
156157
Py_XSETREF(*p, newitem);
157158
return 0;
158159
}
159160

161+
#if 0 // GraalPy change
160162
void
161163
_PyTuple_MaybeUntrack(PyObject *op)
162164
{
@@ -1330,30 +1332,40 @@ void PyTruffle_Tuple_Dealloc(PyTupleObject* self) {
13301332
Py_TYPE(self)->tp_free((PyObject *)self);
13311333
}
13321334

1333-
/*
1334-
* Unsafe variant of PyTuple_GetItem for implementing access macro
1335-
* PyTuple_GET_ITEM.
1336-
*/
1337-
PyObject*
1338-
_PyTuple_GET_ITEM(PyObject* a, Py_ssize_t b) {
1335+
PyObject **
1336+
PyTruffleTuple_GetItems(PyObject *op)
1337+
{
13391338
#ifdef GRAALVM_PYTHON_LLVM_MANAGED
1340-
return GraalPyTruffleTuple_GetItem(a, b);
1339+
return GraalPy_get_PyTupleObject_ob_item(op);
13411340
#else /* GRAALVM_PYTHON_LLVM_MANAGED */
13421341
PyObject *res;
13431342
PyObject **ob_item;
1344-
if (points_to_py_handle_space(a)) {
1345-
GraalPyVarObject *ptr = (GraalPyVarObject *) pointer_to_stub(a);
1343+
if (points_to_py_handle_space(op)) {
1344+
GraalPyVarObject *ptr = (GraalPyVarObject *) pointer_to_stub(op);
13461345
ob_item = ((GraalPyVarObject *) ptr)->ob_item;
13471346
/* The UNLIKELY is maybe not true but the branch is costly anyway. So,
13481347
if we can optimize for something, it should be the path without the
13491348
upcall. */
13501349
if (UNLIKELY(ob_item == NULL)) {
1351-
ptr->ob_item = (ob_item = GraalPy_get_PyTupleObject_ob_item((PyTupleObject *)a));
1350+
ptr->ob_item = (ob_item = GraalPy_get_PyTupleObject_ob_item((PyTupleObject *)op));
13521351
}
13531352
} else {
1354-
ob_item = ((PyTupleObject *) a)->ob_item;
1353+
ob_item = ((PyTupleObject *) op)->ob_item;
13551354
}
13561355
assert(ob_item != NULL);
1357-
return ob_item[b];
1356+
return ob_item;
1357+
#endif /* GRAALVM_PYTHON_LLVM_MANAGED */
1358+
}
1359+
1360+
/*
1361+
* Unsafe variant of PyTuple_GetItem for implementing access macro
1362+
* PyTuple_GET_ITEM.
1363+
*/
1364+
PyObject*
1365+
_PyTuple_GET_ITEM(PyObject* a, Py_ssize_t b) {
1366+
#ifdef GRAALVM_PYTHON_LLVM_MANAGED
1367+
return GraalPyTruffleTuple_GetItem(a, b);
1368+
#else /* GRAALVM_PYTHON_LLVM_MANAGED */
1369+
return PyTruffleTuple_GetItems(a)[b];
13581370
#endif /* GRAALVM_PYTHON_LLVM_MANAGED */
13591371
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextListBuiltins.java

Lines changed: 0 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@
4949
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyObjectBorrowed;
5050
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyObjectTransfer;
5151
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_ssize_t;
52-
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Void;
5352
import static com.oracle.graal.python.nodes.ErrorMessages.BAD_ARG_TO_INTERNAL_FUNC_S;
5453

5554
import java.util.Arrays;
@@ -62,8 +61,6 @@
6261
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiUnaryBuiltinNode;
6362
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.PromoteBorrowedValue;
6463
import com.oracle.graal.python.builtins.objects.PNone;
65-
import com.oracle.graal.python.builtins.objects.cext.PythonAbstractNativeObject;
66-
import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodes;
6764
import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodes.GetItemScalarNode;
6865
import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodes.ListGeneralizationNode;
6966
import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodes.SetItemScalarNode;
@@ -76,20 +73,16 @@
7673
import com.oracle.graal.python.nodes.ErrorMessages;
7774
import com.oracle.graal.python.nodes.PRaiseNode;
7875
import com.oracle.graal.python.nodes.builtins.ListNodes.AppendNode;
79-
import com.oracle.graal.python.nodes.builtins.ListNodes.GetNativeListStorage;
8076
import com.oracle.graal.python.nodes.builtins.TupleNodes.ConstructTupleNode;
8177
import com.oracle.graal.python.runtime.object.PythonObjectFactory;
82-
import com.oracle.graal.python.runtime.sequence.storage.NativeSequenceStorage;
8378
import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
8479
import com.oracle.graal.python.util.PythonUtils;
8580
import com.oracle.truffle.api.dsl.Bind;
8681
import com.oracle.truffle.api.dsl.Cached;
87-
import com.oracle.truffle.api.dsl.Cached.Exclusive;
8882
import com.oracle.truffle.api.dsl.Cached.Shared;
8983
import com.oracle.truffle.api.dsl.Fallback;
9084
import com.oracle.truffle.api.dsl.Specialization;
9185
import com.oracle.truffle.api.nodes.Node;
92-
import com.oracle.truffle.api.profiles.InlinedConditionProfile;
9386

9487
public final class PythonCextListBuiltins {
9588

@@ -285,71 +278,6 @@ int fallback(Object list, @SuppressWarnings("unused") Object i, @SuppressWarning
285278
}
286279
}
287280

288-
@CApiBuiltin(ret = Int, args = {PyObject, Py_ssize_t, PyObjectTransfer}, call = Direct)
289-
abstract static class PyList_SetItem extends CApiTernaryBuiltinNode {
290-
@Specialization
291-
int doManaged(PList list, Object position, Object element,
292-
@Bind("this") Node inliningTarget,
293-
@Cached("createForList()") SequenceStorageNodes.SetItemNode setItemNode,
294-
@Cached InlinedConditionProfile generalizedProfile) {
295-
SequenceStorage newStorage = setItemNode.execute(null, list.getSequenceStorage(), position, element);
296-
if (generalizedProfile.profile(inliningTarget, list.getSequenceStorage() != newStorage)) {
297-
list.setSequenceStorage(newStorage);
298-
}
299-
return 0;
300-
}
301-
302-
@Fallback
303-
int fallback(Object list, @SuppressWarnings("unused") Object i, @SuppressWarnings("unused") Object item) {
304-
throw raiseFallback(list, PythonBuiltinClassType.PList);
305-
}
306-
}
307-
308-
@CApiBuiltin(ret = Void, args = {PyObject, Py_ssize_t, PyObject}, call = Direct)
309-
abstract static class PyTruffleList_SET_ITEM extends CApiTernaryBuiltinNode {
310-
@Specialization
311-
static Object doManaged(PList list, long index, Object element,
312-
@Bind("this") Node inliningTarget,
313-
@Cached ListGeneralizationNode generalizationNode,
314-
@Cached SequenceStorageNodes.InitializeItemScalarNode setItemNode,
315-
@Cached InlinedConditionProfile generalizedProfile,
316-
@Exclusive @Cached PRaiseNode.Lazy raiseNode) {
317-
SequenceStorage sequenceStorage = list.getSequenceStorage();
318-
checkBounds(inliningTarget, sequenceStorage, index, raiseNode);
319-
SequenceStorage newStorage = generalizationNode.execute(inliningTarget, sequenceStorage, element);
320-
setItemNode.execute(inliningTarget, newStorage, (int) index, element);
321-
if (generalizedProfile.profile(inliningTarget, list.getSequenceStorage() != newStorage)) {
322-
list.setSequenceStorage(newStorage);
323-
}
324-
return PNone.NO_VALUE;
325-
}
326-
327-
@Specialization
328-
static Object doNative(PythonAbstractNativeObject list, long index, Object element,
329-
@Bind("this") Node inliningTarget,
330-
@Cached GetNativeListStorage asNativeStorage,
331-
@Cached SequenceStorageNodes.InitializeNativeItemScalarNode setItemNode,
332-
@Exclusive @Cached PRaiseNode.Lazy raiseNode) {
333-
NativeSequenceStorage sequenceStorage = asNativeStorage.execute(list);
334-
checkBounds(inliningTarget, sequenceStorage, index, raiseNode);
335-
setItemNode.execute(sequenceStorage, (int) index, element);
336-
return PNone.NO_VALUE;
337-
}
338-
339-
@Fallback
340-
@SuppressWarnings("unused")
341-
Object fallback(Object list, Object index, Object element) {
342-
throw raiseFallback(list, PythonBuiltinClassType.PList);
343-
}
344-
345-
private static void checkBounds(Node inliningTarget, SequenceStorage sequenceStorage, long index, PRaiseNode.Lazy raiseNode) {
346-
// we must do a bounds-check but we must not normalize the index
347-
if (index < 0 || index >= sequenceStorage.length()) {
348-
throw raiseNode.get(inliningTarget).raise(IndexError, ErrorMessages.INDEX_OUT_OF_BOUNDS);
349-
}
350-
}
351-
}
352-
353281
@CApiBuiltin(ret = Int, args = {PyObject}, call = Direct)
354282
abstract static class PyList_Reverse extends CApiUnaryBuiltinNode {
355283
@Specialization

0 commit comments

Comments
 (0)