Skip to content

Commit e47ee57

Browse files
steve-ssteve-s
committed
[GR-65765] Remove the ObjectHashMap side effecting keys flag.
PullRequest: graalpython/3795
2 parents 8128d18 + 85395b7 commit e47ee57

File tree

11 files changed

+108
-212
lines changed

11 files changed

+108
-212
lines changed

‎graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/objects/ObjectHashMapTests.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@
6767
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap;
6868
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.MapCursor;
6969
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PopNode;
70-
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PutUnsafeNode;
70+
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PutNode;
7171
import com.oracle.graal.python.lib.PyObjectHashNode;
7272
import com.oracle.graal.python.lib.PyObjectRichCompareBool;
7373
import com.oracle.graal.python.lib.RichCmpOp;
@@ -101,7 +101,7 @@ public boolean execute(Frame frame, Node inliningTarget, Object a, Object b, Ric
101101

102102
@Test
103103
public void testCollisionsByPuttingManyKeysWithSameHash() {
104-
ObjectHashMap map = new ObjectHashMap(true);
104+
ObjectHashMap map = new ObjectHashMap();
105105
LinkedHashMap<DictKey, Object> expected = new LinkedHashMap<>();
106106
for (int i = 0; i < 100; i++) {
107107
DictKey key = new DictKey(42);
@@ -127,7 +127,7 @@ public void testCollisionsByPuttingManyKeysWithSameHash() {
127127

128128
@Test
129129
public void testCollisionsByPuttingAndRemovingTheSameKey() {
130-
ObjectHashMap map = new ObjectHashMap(true);
130+
ObjectHashMap map = new ObjectHashMap();
131131
LinkedHashMap<DictKey, Object> expected = new LinkedHashMap<>();
132132
DictKey key = new DictKey(42);
133133
for (int i = 0; i < 100; i++) {
@@ -144,7 +144,7 @@ public void testCollisionsByPuttingAndRemovingTheSameKey() {
144144

145145
@Test
146146
public void testCollisionsByPuttingAndRemovingTheSameKeys() {
147-
ObjectHashMap map = new ObjectHashMap(true);
147+
ObjectHashMap map = new ObjectHashMap();
148148
LinkedHashMap<DictKey, Object> expected = new LinkedHashMap<>();
149149
DictKey[] keys = new DictKey[]{new DictKey(42), new DictKey(1)};
150150
for (int i = 0; i < 100; i++) {
@@ -163,7 +163,7 @@ public void testCollisionsByPuttingAndRemovingTheSameKeys() {
163163

164164
@Test
165165
public void testLongHashMapStressTest() {
166-
ObjectHashMap map = new ObjectHashMap(true);
166+
ObjectHashMap map = new ObjectHashMap();
167167

168168
// put/remove many random (with fixed seed) keys, check consistency against LinkedHashMap
169169
testBasics(map);
@@ -374,7 +374,7 @@ private static void remove(ObjectHashMap map, Object key, long hash) {
374374

375375
private static void put(ObjectHashMap map, Object key, long hash, Object value) {
376376
InlinedCountingConditionProfile uncachedCounting = InlinedCountingConditionProfile.getUncached();
377-
PutUnsafeNode.doPutWithRestart(null, null, map, key, hash, value,
377+
PutNode.doPutWithRestart(null, null, map, key, hash, value,
378378
InlinedBranchProfile.getUncached(), uncachedCounting, uncachedCounting, uncachedCounting,
379379
uncachedCounting, InlinedBranchProfile.getUncached(), InlinedBranchProfile.getUncached(),
380380
new EqNodeStub());

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

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Copyright (c) 2021, 2021, Oracle and/or its affiliates.
1+
# Copyright (c) 2021, 2025, Oracle and/or its affiliates.
22
# Copyright (C) 1996-2020 Python Software Foundation
33
#
44
# Licensed under the PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2
@@ -175,28 +175,58 @@ class Z():
175175

176176
def test_mro_change_on_attr_access():
177177
eq_called = []
178-
class MyKey(object):
179-
def __hash__(self):
178+
class MyKey(object):
179+
def __hash__(self):
180180
return hash('mykey')
181181
def __eq__(self, other):
182182
eq_called.append(1)
183183
X.__bases__ = (Base2,)
184184

185185
class Base(object):
186186
mykey = 'base 42'
187+
def __str__(self): return 'Base'
187188

188189
class Base2(object):
189190
mykey = 'base2 42'
191+
def __str__(self): return 'Base2'
190192

191193
X = type('X', (Base,), {MyKey(): 5})
192194
assert X.mykey == 'base 42'
193195
assert eq_called == [1]
196+
197+
# Note: CPython does not invalidate the lookup cached on the type in __bases__ setter
198+
assert X.mykey in ['base 42', 'base2 42']
199+
assert eq_called in [[1], [1, 1]]
200+
201+
# Now try this on the object
202+
eq_called = []
203+
X = type('X', (Base,), {MyKey(): 5})
204+
xobj = X()
205+
assert str(xobj) == 'Base'
206+
assert xobj.mykey == 'base 42'
207+
assert eq_called == [1]
208+
assert str(xobj) == 'Base2', str(xobj) # slots are invalidated
209+
210+
# CPython does not invalidate the lookup cached on the type,
211+
# so we check that it's one or the other
212+
assert xobj.mykey in ['base 42', 'base2 42']
213+
assert eq_called in [[1], [1, 1]]
214+
original_eq_called_len = len(eq_called)
215+
216+
# This invalidates the lookup even on CPython, we should see __eq__ call and the new value
217+
X.anotherkey = 'bogus'
218+
assert xobj.mykey in 'base2 42'
219+
assert len(eq_called) in [original_eq_called_len, original_eq_called_len + 1]
220+
221+
assert X.mykey == 'base2 42'
194222

195223
# ----------------------------------
224+
eq_called = []
196225
class MyKey(object):
197226
def __hash__(self):
198227
return hash('mykey')
199228
def __eq__(self, other):
229+
eq_called.append(1)
200230
X.__bases__ = (Base,)
201231

202232
class Base(object):
@@ -208,11 +238,15 @@ class Base2(object):
208238
X = type('X', (Base,Base2,), {MyKey(): 5})
209239
mk = X.mykey
210240
assert mk == '42'
211-
241+
assert eq_called == [1]
242+
243+
eq_called = []
212244
X = type('X', (Base2,), {MyKey(): 5})
213245
assert X.mykey == '42'
246+
assert eq_called == [1]
214247

215248
# ----------------------------------
249+
eq_called = []
216250
class Base(object):
217251
mykey = 'from Base2'
218252

@@ -223,7 +257,7 @@ class Base2(object):
223257
try:
224258
assert X.mykey == '42'
225259
except AttributeError as e:
226-
assert True
260+
assert eq_called == [1]
227261
else:
228262
assert False
229263

@@ -338,4 +372,4 @@ class C(B):
338372
pass
339373

340374
c = C()
341-
assert c.foo() == 42
375+
assert c.foo() == 42

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

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@
5050
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.DictKey;
5151
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.MapCursor;
5252
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PutNode;
53-
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PutUnsafeNode;
5453
import com.oracle.graal.python.lib.PyObjectHashNode;
5554
import com.oracle.truffle.api.CompilerAsserts;
5655
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
@@ -73,21 +72,21 @@ public static EconomicMapStorage create() {
7372
}
7473

7574
public static EconomicMapStorage createWithSideEffects() {
76-
return new EconomicMapStorage(4, true);
75+
return new EconomicMapStorage(4);
7776
}
7877

7978
public static EconomicMapStorage create(int initialCapacity) {
80-
return new EconomicMapStorage(initialCapacity, false);
79+
return new EconomicMapStorage(initialCapacity);
8180
}
8281

8382
final ObjectHashMap map;
8483

85-
private EconomicMapStorage(int initialCapacity, boolean hasSideEffects) {
86-
this.map = new ObjectHashMap(initialCapacity, hasSideEffects);
84+
private EconomicMapStorage(int initialCapacity) {
85+
this.map = new ObjectHashMap(initialCapacity);
8786
}
8887

8988
private EconomicMapStorage() {
90-
this(4, false);
89+
this(4);
9190
}
9291

9392
public EconomicMapStorage(ObjectHashMap original, boolean copy) {
@@ -96,14 +95,14 @@ public EconomicMapStorage(ObjectHashMap original, boolean copy) {
9695

9796
@TruffleBoundary
9897
public static EconomicMapStorage create(LinkedHashMap<String, Object> map) {
99-
EconomicMapStorage result = new EconomicMapStorage(map.size(), false);
98+
EconomicMapStorage result = new EconomicMapStorage(map.size());
10099
putAllUncached(map, result);
101100
return result;
102101
}
103102

104103
public static EconomicMapStorage createGeneric(LinkedHashMap<Object, Object> map) {
105104
CompilerAsserts.neverPartOfCompilation();
106-
EconomicMapStorage result = new EconomicMapStorage(map.size(), false);
105+
EconomicMapStorage result = new EconomicMapStorage(map.size());
107106
putAllUncachedGeneric(map, result);
108107
return result;
109108
}
@@ -140,7 +139,7 @@ public HashingStorage copy() {
140139
return new EconomicMapStorage(this.map, true);
141140
}
142141

143-
protected void setValueForAllKeys(VirtualFrame frame, Node inliningTarget, Object value, PutUnsafeNode putNode, InlinedLoopConditionProfile loopProfile) {
142+
protected void setValueForAllKeys(VirtualFrame frame, Node inliningTarget, Object value, ObjectHashMap.PutNode putNode, InlinedLoopConditionProfile loopProfile) {
144143
MapCursor cursor = map.getEntries();
145144
final int size = map.size();
146145
loopProfile.profileCounted(inliningTarget, size);
@@ -152,7 +151,7 @@ protected void setValueForAllKeys(VirtualFrame frame, Node inliningTarget, Objec
152151

153152
@TruffleBoundary
154153
public void putUncached(TruffleString key, Object value) {
155-
PutUnsafeNode.putUncached(this.map, key, PyObjectHashNode.hash(key, HashCodeNode.getUncached()), value);
154+
ObjectHashMap.PutNode.putUncached(this.map, key, PyObjectHashNode.hash(key, HashCodeNode.getUncached()), value);
156155
}
157156

158157
@TruffleBoundary
@@ -162,7 +161,7 @@ public void putUncached(Object key, Object value) {
162161

163162
@TruffleBoundary
164163
public void putUncached(int key, Object value) {
165-
PutUnsafeNode.putUncached(this.map, key, PyObjectHashNode.hash(key), value);
164+
ObjectHashMap.PutNode.putUncached(this.map, key, PyObjectHashNode.hash(key), value);
166165
}
167166

168167
@TruffleBoundary
@@ -214,7 +213,7 @@ public abstract static class EconomicMapSetStringKey extends SpecializedSetStrin
214213
@Specialization
215214
static void doIt(Node inliningTarget, HashingStorage self, TruffleString key, Object value,
216215
@Cached PyObjectHashNode hashNode,
217-
@Cached PutUnsafeNode putNode) {
216+
@Cached ObjectHashMap.PutNode putNode) {
218217
putNode.put(null, inliningTarget, ((EconomicMapStorage) self).map, key, hashNode.execute(null, inliningTarget, key), value);
219218
}
220219
}

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,15 +43,15 @@
4343
import static com.oracle.graal.python.util.PythonUtils.TS_ENCODING;
4444

4545
import com.oracle.graal.python.builtins.objects.PNone;
46-
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodesFactory.GetClonedHashingStorageNodeGen;
47-
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodesFactory.SetItemNodeGen;
4846
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodes.HashingStorageCopy;
4947
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodes.HashingStorageGetIterator;
5048
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodes.HashingStorageIterator;
5149
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodes.HashingStorageIteratorKey;
5250
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodes.HashingStorageIteratorNext;
5351
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodes.HashingStorageSetItem;
54-
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PutUnsafeNode;
52+
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PutNode;
53+
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodesFactory.GetClonedHashingStorageNodeGen;
54+
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodesFactory.SetItemNodeGen;
5555
import com.oracle.graal.python.builtins.objects.dict.DictNodes;
5656
import com.oracle.graal.python.builtins.objects.dict.PDict;
5757
import com.oracle.graal.python.builtins.objects.dict.PDictView;
@@ -123,7 +123,7 @@ abstract static class SetValueHashingStorageNode extends PNodeWithContext {
123123

124124
@Specialization
125125
static HashingStorage doEconomicStorage(VirtualFrame frame, Node inliningTarget, EconomicMapStorage map, Object value,
126-
@Shared("putNode") @Cached PutUnsafeNode putNode,
126+
@Shared("putNode") @Cached ObjectHashMap.PutNode putNode,
127127
@Shared("loopProfile") @Cached InlinedLoopConditionProfile loopProfile) {
128128
// We want to avoid calling __hash__() during map.put
129129
map.setValueForAllKeys(frame, inliningTarget, value, putNode, loopProfile);
@@ -137,7 +137,7 @@ static HashingStorage doGeneric(VirtualFrame frame, Node inliningTarget, Hashing
137137
@Cached HashingStorageGetIterator getIterator,
138138
@Cached HashingStorageIteratorNext itNext,
139139
@Cached HashingStorageIteratorKey itKey,
140-
@Shared("putNode") @Cached PutUnsafeNode putNode,
140+
@Shared("putNode") @Cached PutNode putNode,
141141
@Shared("loopProfile") @Cached InlinedLoopConditionProfile loopProfile) {
142142
HashingStorageIterator it = getIterator.execute(inliningTarget, map);
143143
while (itNext.execute(inliningTarget, map, it)) {

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

Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@
6262
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodesFactory.HashingStorageSetItemWithHashNodeGen;
6363
import com.oracle.graal.python.builtins.objects.common.KeywordsStorage.GetKeywordsStorageItemNode;
6464
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PutNode;
65-
import com.oracle.graal.python.builtins.objects.common.ObjectHashMap.PutUnsafeNode;
6665
import com.oracle.graal.python.lib.PyObjectHashNode;
6766
import com.oracle.graal.python.lib.PyObjectRichCompareBool;
6867
import com.oracle.graal.python.lib.PyUnicodeCheckExactNode;
@@ -104,28 +103,6 @@
104103

105104
public class HashingStorageNodes {
106105

107-
public abstract static class HashingStorageGuards {
108-
private HashingStorageGuards() {
109-
}
110-
111-
/**
112-
* If the storage may contain keys that may have side-effecting {@code __eq__}
113-
* implementation.
114-
*/
115-
public static boolean mayHaveSideEffectingEq(PHashingCollection wrapper) {
116-
return mayHaveSideEffectingEq(wrapper.getDictStorage());
117-
}
118-
119-
public static boolean mayHaveSideEffectingEq(HashingStorage storage) {
120-
return storage instanceof EconomicMapStorage;
121-
}
122-
123-
public static boolean mayHaveSideEffects(PHashingCollection wrapper) {
124-
HashingStorage s = wrapper.getDictStorage();
125-
return s instanceof EconomicMapStorage && ((EconomicMapStorage) s).map.hasSideEffectingKeys();
126-
}
127-
}
128-
129106
@GenerateUncached
130107
@GenerateInline
131108
@GenerateCached(false)
@@ -253,7 +230,7 @@ abstract static class SpecializedSetStringKey extends Node {
253230
public abstract void execute(Node inliningTarget, HashingStorage self, TruffleString key, Object value);
254231
}
255232

256-
static EconomicMapStorage dynamicObjectStorageToEconomicMap(Node inliningTarget, DynamicObjectStorage s, DynamicObjectLibrary dylib, PyObjectHashNode hashNode, PutUnsafeNode putNode) {
233+
static EconomicMapStorage dynamicObjectStorageToEconomicMap(Node inliningTarget, DynamicObjectStorage s, DynamicObjectLibrary dylib, PyObjectHashNode hashNode, ObjectHashMap.PutNode putNode) {
257234
// TODO: shouldn't we invalidate all MRO assumptions in this case?
258235
DynamicObject store = s.store;
259236
EconomicMapStorage result = EconomicMapStorage.create(dylib.getShape(store).getPropertyCount());
@@ -263,7 +240,7 @@ static EconomicMapStorage dynamicObjectStorageToEconomicMap(Node inliningTarget,
263240
if (k instanceof TruffleString) {
264241
Object v = dylib.getOrDefault(store, k, PNone.NO_VALUE);
265242
if (v != PNone.NO_VALUE) {
266-
putNode.execute(null, inliningTarget, resultMap, k, hashNode.execute(null, inliningTarget, k), v);
243+
putNode.put(null, inliningTarget, resultMap, k, hashNode.execute(null, inliningTarget, k), v);
267244
}
268245
}
269246
}
@@ -365,7 +342,7 @@ static HashingStorage domStringKey(Node inliningTarget, DynamicObjectStorage sel
365342
static HashingStorage domTransition(Frame frame, Node inliningTarget, DynamicObjectStorage self, Object key, @SuppressWarnings("unused") long keyHash, Object value,
366343
@SuppressWarnings("unused") boolean transition, DynamicObjectLibrary dylib,
367344
@Cached PyObjectHashNode hashNode,
368-
@Cached PutUnsafeNode putUnsafeNode,
345+
@Cached ObjectHashMap.PutNode putUnsafeNode,
369346
@Cached PutNode putNode) {
370347
EconomicMapStorage result = dynamicObjectStorageToEconomicMap(inliningTarget, self, dylib, hashNode, putUnsafeNode);
371348
putNode.execute(frame, inliningTarget, result.map, key, keyHash, value);
@@ -491,7 +468,7 @@ static HashingStorage domStringKey(Node inliningTarget, DynamicObjectStorage sel
491468
static HashingStorage domTransition(Frame frame, Node inliningTarget, DynamicObjectStorage self, Object key, Object value,
492469
@SuppressWarnings("unused") boolean transition, DynamicObjectLibrary dylib,
493470
@Cached PyObjectHashNode hashNode,
494-
@Cached PutUnsafeNode putUnsafeNode,
471+
@Cached ObjectHashMap.PutNode putUnsafeNode,
495472
@Cached PutNode putNode) {
496473
EconomicMapStorage result = dynamicObjectStorageToEconomicMap(inliningTarget, self, dylib, hashNode, putUnsafeNode);
497474
putNode.execute(frame, inliningTarget, result.map, key, hashNode.execute(frame, inliningTarget, key), value);

0 commit comments

Comments
 (0)