Skip to content

Commit 69e88ef

Browse files
msimacekmsimacek
committed
[GR-61531] Fix native storage refcounting
PullRequest: graalpython/3655
2 parents 335ffd2 + 7d6c595 commit 69e88ef

File tree

3 files changed

+159
-10
lines changed

3 files changed

+159
-10
lines changed

‎graalpython/com.oracle.graal.python.test/src/tests/test_list.py

Lines changed: 110 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Copyright (c) 2018, 2024, Oracle and/or its affiliates.
1+
# Copyright (c) 2018, 2025, Oracle and/or its affiliates.
22
# Copyright (C) 1996-2017 Python Software Foundation
33
#
44
# Licensed under the PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2
@@ -860,5 +860,114 @@ def test_clear(self):
860860
with self.assertRaisesRegex(TypeError, "list"):
861861
list.clear(42)
862862

863+
864+
class TestObject:
865+
def __init__(self, name):
866+
self.name = name
867+
868+
def __repr__(self):
869+
return self.name
870+
871+
872+
class NativeStorageTests(unittest.TestCase):
873+
def setUp(self):
874+
self.o1 = TestObject('o1')
875+
self.o2 = TestObject('o2')
876+
self.o3 = TestObject('o3')
877+
self.o4 = TestObject('o4')
878+
self.list = [self.o1, self.o2, self.o3]
879+
storage_to_native(self.list)
880+
881+
def assert_refcount(self, o, expected_refcount):
882+
expected_refcount += 10 if sys.implementation.name == 'graalpy' else 3
883+
refcount = sys.getrefcount(o)
884+
if refcount == -1 and sys.implementation.name == 'graalpy':
885+
refcount = 10
886+
self.assertEqual(refcount, expected_refcount,
887+
f"Expected {o} to have refcount {expected_refcount}, was {refcount}")
888+
889+
def check_refcounts(self, o1=1, o2=1, o3=1, o4=0):
890+
self.assert_refcount(self.o1, o1)
891+
self.assert_refcount(self.o2, o2)
892+
self.assert_refcount(self.o3, o3)
893+
self.assert_refcount(self.o4, o4)
894+
895+
def test_get(self):
896+
self.assertEqual(self.list[1], self.o2)
897+
self.assertEqual(self.list[-1], self.o3)
898+
self.assertEqual(self.list[1:], [self.o2, self.o3])
899+
900+
def test_set_del(self):
901+
# n.b. it's important to test the setting and deleting operations together to properly test interactions between
902+
# shrinking and then growing the same storage
903+
self.list[1] = self.o4
904+
self.assertEqual(self.list, [self.o1, self.o4, self.o3])
905+
self.check_refcounts(o1=1, o2=0, o3=1, o4=1)
906+
self.list[1:] = [self.o2, self.o3, self.o4]
907+
self.assertEqual(self.list, [self.o1, self.o2, self.o3, self.o4])
908+
self.check_refcounts(o1=1, o2=1, o3=1, o4=1)
909+
del self.list[1]
910+
self.assertEqual(self.list, [self.o1, self.o3, self.o4])
911+
self.check_refcounts(o1=1, o2=0, o3=1, o4=1)
912+
self.list[1:1] = [self.o2]
913+
self.assertEqual(self.list, [self.o1, self.o2, self.o3, self.o4])
914+
self.check_refcounts(o1=1, o2=1, o3=1, o4=1)
915+
del self.list[1:3]
916+
self.assertEqual(self.list, [self.o1, self.o4])
917+
self.check_refcounts(o1=1, o2=0, o3=0, o4=1)
918+
self.list[1:] = [self.o2, self.o3]
919+
self.assertEqual(self.list, [self.o1, self.o2, self.o3])
920+
self.check_refcounts(o1=1, o2=1, o3=1, o4=0)
921+
922+
def test_insert_remove(self):
923+
self.list.remove(self.o3)
924+
self.assertEqual(self.list, [self.o1, self.o2])
925+
self.check_refcounts(o1=1, o2=1, o3=0, o4=0)
926+
self.list.insert(0, self.o4)
927+
self.assertEqual(self.list, [self.o4, self.o1, self.o2])
928+
self.check_refcounts(o1=1, o2=1, o3=0, o4=1)
929+
930+
def test_append_pop(self):
931+
self.list.pop(1)
932+
self.assertEqual(self.list, [self.o1, self.o3])
933+
self.check_refcounts(o1=1, o2=0, o3=1, o4=0)
934+
self.list.append(self.o4)
935+
self.assertEqual(self.list, [self.o1, self.o3, self.o4])
936+
self.check_refcounts(o1=1, o2=0, o3=1, o4=1)
937+
938+
def test_extend(self):
939+
self.list.extend([self.o4, self.o1])
940+
self.assertEqual(self.list, [self.o1, self.o2, self.o3, self.o4, self.o1])
941+
self.check_refcounts(o1=2, o2=1, o3=1, o4=1)
942+
del self.list[1:]
943+
self.assertEqual(self.list, [self.o1])
944+
self.check_refcounts(o1=1, o2=0, o3=0, o4=0)
945+
self.list += [self.o2, self.o3]
946+
self.assertEqual(self.list, [self.o1, self.o2, self.o3])
947+
self.check_refcounts(o1=1, o2=1, o3=1, o4=0)
948+
949+
def test_inplace_repeat(self):
950+
self.list *= 2
951+
self.assertEqual(self.list, [self.o1, self.o2, self.o3, self.o1, self.o2, self.o3])
952+
# We don't actually do it in place, we make a new managed storage
953+
# self.check_refcounts(o1=2, o2=2, o3=2, o4=0)
954+
955+
def test_sort(self):
956+
self.list.sort(key=lambda x: x.name, reverse=True)
957+
self.assertEqual(self.list, [self.o3, self.o2, self.o1])
958+
self.check_refcounts(o1=1, o2=1, o3=1, o4=0)
959+
960+
def test_reverse(self):
961+
self.list.reverse()
962+
self.assertEqual(self.list, [self.o3, self.o2, self.o1])
963+
self.check_refcounts(o1=1, o2=1, o3=1, o4=0)
964+
965+
def test_clear(self):
966+
self.list.clear()
967+
self.assertEqual(self.list, [])
968+
# We create a new storage and let the old be GC'ed
969+
# self.check_refcounts(o1=0, o2=0, o3=0, o4=0)
970+
971+
863972
if __name__ == '__main__':
864973
unittest.main()

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/SequenceStorageNodes.java

Lines changed: 44 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, 2024, Oracle and/or its affiliates.
2+
* Copyright (c) 2018, 2025, Oracle and/or its affiliates.
33
* Copyright (c) 2013, Regents of the University of California
44
*
55
* All rights reserved.
@@ -124,15 +124,15 @@
124124
import com.oracle.graal.python.runtime.sequence.storage.LongSequenceStorage;
125125
import com.oracle.graal.python.runtime.sequence.storage.MroSequenceStorage;
126126
import com.oracle.graal.python.runtime.sequence.storage.NativeByteSequenceStorage;
127+
import com.oracle.graal.python.runtime.sequence.storage.NativeIntSequenceStorage;
127128
import com.oracle.graal.python.runtime.sequence.storage.NativeObjectSequenceStorage;
129+
import com.oracle.graal.python.runtime.sequence.storage.NativePrimitiveSequenceStorage;
128130
import com.oracle.graal.python.runtime.sequence.storage.NativeSequenceStorage;
129131
import com.oracle.graal.python.runtime.sequence.storage.ObjectSequenceStorage;
130132
import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
131133
import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage.StorageType;
132134
import com.oracle.graal.python.runtime.sequence.storage.SequenceStorageFactory;
133135
import com.oracle.graal.python.runtime.sequence.storage.SequenceStoreException;
134-
import com.oracle.graal.python.runtime.sequence.storage.NativePrimitiveSequenceStorage;
135-
import com.oracle.graal.python.runtime.sequence.storage.NativeIntSequenceStorage;
136136
import com.oracle.graal.python.util.BiFunction;
137137
import com.oracle.graal.python.util.OverflowException;
138138
import com.oracle.graal.python.util.PythonUtils;
@@ -167,6 +167,7 @@
167167
import com.oracle.truffle.api.profiles.InlinedExactClassProfile;
168168
import com.oracle.truffle.api.profiles.InlinedLoopConditionProfile;
169169
import com.oracle.truffle.api.strings.TruffleString;
170+
170171
import sun.misc.Unsafe;
171172

172173
public abstract class SequenceStorageNodes {
@@ -3492,12 +3493,14 @@ public abstract static class SetNativeLenNode extends Node {
34923493
static void doShrink(NativeObjectSequenceStorage s, int len,
34933494
@Bind("this") Node inliningTarget,
34943495
@Cached CStructAccess.ReadPointerNode readNode,
3496+
@Cached CStructAccess.WritePointerNode writeNode,
34953497
@Cached CExtNodes.XDecRefPointerNode decRefPointerNode) {
34963498
if (len < s.length()) {
34973499
// When shrinking, we need to decref the items that are now past the end
34983500
for (int i = len; i < s.length(); i++) {
34993501
Object elementPointer = readNode.readArrayElement(s.getPtr(), i);
35003502
decRefPointerNode.execute(inliningTarget, elementPointer);
3503+
writeNode.writeArrayElement(s.getPtr(), i, 0L);
35013504
}
35023505
}
35033506
s.setNewLength(len);
@@ -3594,7 +3597,7 @@ static void doLastItem(Node inliningTarget, SequenceStorage s, @SuppressWarnings
35943597
setLenNode.execute(inliningTarget, s, s.length() - 1);
35953598
}
35963599

3597-
@Specialization(guards = "!isForeignSequenceStorage(s)")
3600+
@Specialization(guards = {"!isNativeObjectStorage(s)", "!isForeignSequenceStorage(s)"})
35983601
static void doGeneric(Node inliningTarget, SequenceStorage s, int idx,
35993602
@Cached GetItemScalarNode getItemNode,
36003603
@Cached SetItemScalarNode setItemNode,
@@ -3607,6 +3610,21 @@ static void doGeneric(Node inliningTarget, SequenceStorage s, int idx,
36073610
setLenNode.execute(inliningTarget, s, len - 1);
36083611
}
36093612

3613+
@Specialization
3614+
static void doNativeObjectStorage(Node inliningTarget, NativeObjectSequenceStorage s, int idx,
3615+
@Cached(inline = false) CStructAccess.ReadPointerNode readPointerNode,
3616+
@Cached(inline = false) CStructAccess.WritePointerNode writePointerNode,
3617+
@Cached CExtNodes.XDecRefPointerNode decRefNode) {
3618+
int len = s.length();
3619+
Object deleted = readPointerNode.readArrayElement(s.getPtr(), idx);
3620+
for (int i = idx; i < len - 1; i++) {
3621+
writePointerNode.writeArrayElement(s.getPtr(), i, readPointerNode.readArrayElement(s.getPtr(), i + 1));
3622+
}
3623+
writePointerNode.writeArrayElement(s.getPtr(), len - 1, 0L);
3624+
s.setNewLength(len - 1);
3625+
decRefNode.execute(inliningTarget, deleted);
3626+
}
3627+
36103628
@Specialization
36113629
static void doForeign(Node inliningTarget, ForeignSequenceStorage s, int idx,
36123630
@Cached ForeignSequenceStorage.RemoveNode removeNode) {
@@ -4067,16 +4085,33 @@ static SequenceStorage doNativeInt(Node inliningTarget, NativeIntSequenceStorage
40674085
}
40684086

40694087
@Specialization
4070-
protected static SequenceStorage doNativeStorage(Node inliningTarget, NativeSequenceStorage storage, int index, Object value,
4088+
protected static SequenceStorage doNativeObjectStorage(Node inliningTarget, NativeObjectSequenceStorage storage, int index, Object value,
40714089
@Exclusive @Cached EnsureCapacityNode ensureCapacityNode,
4072-
@Cached(inline = false) GetItemScalarNode getItem,
4073-
@Cached SetItemScalarNode setItem) {
4090+
@Cached(inline = false) CStructAccess.ReadPointerNode readPointerNode,
4091+
@Cached(inline = false) CStructAccess.WritePointerNode writePointerNode,
4092+
@Cached PythonToNativeNewRefNode toNative) {
40744093
int newLength = storage.length() + 1;
40754094
ensureCapacityNode.execute(inliningTarget, storage, newLength);
40764095
for (int i = storage.length(); i > index; i--) {
4077-
setItem.execute(inliningTarget, storage, i, getItem.execute(inliningTarget, storage, i - 1));
4096+
writePointerNode.writeArrayElement(storage.getPtr(), i, readPointerNode.readArrayElement(storage.getPtr(), i - 1));
40784097
}
4079-
setItem.execute(inliningTarget, storage, index, value);
4098+
writePointerNode.writeArrayElement(storage.getPtr(), index, toNative.execute(value));
4099+
storage.setNewLength(newLength);
4100+
return storage;
4101+
}
4102+
4103+
@Specialization
4104+
protected static SequenceStorage doNativeByteStorage(Node inliningTarget, NativeByteSequenceStorage storage, int index, Object value,
4105+
@Exclusive @Cached EnsureCapacityNode ensureCapacityNode,
4106+
@Cached(inline = false) CStructAccess.ReadByteNode readByteNode,
4107+
@Cached(inline = false) CStructAccess.WriteByteNode writeByteNode,
4108+
@Cached CastToByteNode castToByteNode) {
4109+
int newLength = storage.length() + 1;
4110+
ensureCapacityNode.execute(inliningTarget, storage, newLength);
4111+
for (int i = storage.length(); i > index; i--) {
4112+
writeByteNode.writeArrayElement(storage.getPtr(), i, readByteNode.readArrayElement(storage.getPtr(), i - 1));
4113+
}
4114+
writeByteNode.writeArrayElement(storage.getPtr(), index, castToByteNode.execute(null, value));
40804115
storage.setNewLength(newLength);
40814116
return storage;
40824117
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/PGuards.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@
113113
import com.oracle.graal.python.runtime.sequence.storage.ForeignSequenceStorage;
114114
import com.oracle.graal.python.runtime.sequence.storage.IntSequenceStorage;
115115
import com.oracle.graal.python.runtime.sequence.storage.LongSequenceStorage;
116+
import com.oracle.graal.python.runtime.sequence.storage.NativeObjectSequenceStorage;
116117
import com.oracle.graal.python.runtime.sequence.storage.ObjectSequenceStorage;
117118
import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
118119
import com.oracle.graal.python.util.OverflowException;
@@ -297,6 +298,10 @@ public static boolean isForeignSequenceStorage(SequenceStorage sequenceStorage)
297298
return sequenceStorage instanceof ForeignSequenceStorage;
298299
}
299300

301+
public static boolean isNativeObjectStorage(SequenceStorage sequenceStorage) {
302+
return sequenceStorage instanceof NativeObjectSequenceStorage;
303+
}
304+
300305
public static boolean isList(Object o) {
301306
return o instanceof PList;
302307
}

0 commit comments

Comments
 (0)