Skip to content

Commit 895a103

Browse files
tomasstupkatomasstupka
committed
[GR-30182] OrderedDict wrongly preserves position of removed/reinserted item.
PullRequest: graalpython/1727
2 parents 90ef0cc + a3ef33a commit 895a103

File tree

14 files changed

+46
-95
lines changed

14 files changed

+46
-95
lines changed

‎graalpython/com.oracle.graal.python.test/src/tests/unittest_tags/test_ordered_dict.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
*graalpython.lib-python.3.test.test_ordered_dict.CPythonBuiltinDictTests.test_init
99
*graalpython.lib-python.3.test.test_ordered_dict.CPythonBuiltinDictTests.test_override_update
1010
*graalpython.lib-python.3.test.test_ordered_dict.CPythonBuiltinDictTests.test_popitem
11+
*graalpython.lib-python.3.test.test_ordered_dict.CPythonBuiltinDictTests.test_reinsert
1112
*graalpython.lib-python.3.test.test_ordered_dict.CPythonBuiltinDictTests.test_setitem
1213
*graalpython.lib-python.3.test.test_ordered_dict.CPythonBuiltinDictTests.test_update
1314
*graalpython.lib-python.3.test.test_ordered_dict.CPythonGeneralMappingTests.test_bool

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -766,9 +766,8 @@ abstract static class CharmapBuildNode extends PythonBuiltinNode {
766766
// This is replaced in the core _codecs.py with the full functionality
767767
@Specialization
768768
Object lookup(String chars,
769-
@CachedLanguage PythonLanguage lang,
770769
@CachedLibrary(limit = "3") HashingStorageLibrary lib) {
771-
HashingStorage store = PDict.createNewStorage(lang, false, chars.length());
770+
HashingStorage store = PDict.createNewStorage(false, chars.length());
772771
PDict dict = factory().createDict(store);
773772
int pos = 0;
774773
int num = 0;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ private PCode readCode(ByteBuffer buffer) {
448448

449449
private PDict readDict(int depth, ByteBuffer buffer) {
450450
int len = buffer.getInt();
451-
HashingStorage store = PDict.createNewStorage(PythonLanguage.getCurrent(), false, len);
451+
HashingStorage store = PDict.createNewStorage(false, len);
452452
PDict dict = factory().createDict(store);
453453
for (int i = 0; i < len; i++) {
454454
Object key = readObject(depth + 1, buffer);

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

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,15 +43,14 @@
4343
import java.util.Iterator;
4444
import java.util.NoSuchElementException;
4545

46-
import com.oracle.graal.python.PythonLanguage;
4746
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary.ForEachNode;
4847
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary.HashingStorageIterable;
48+
import com.oracle.graal.python.builtins.objects.dict.PDict;
4949
import com.oracle.graal.python.builtins.objects.function.PArguments.ThreadState;
5050
import com.oracle.graal.python.builtins.objects.object.PythonObjectLibrary;
5151
import com.oracle.truffle.api.dsl.Cached;
5252
import com.oracle.truffle.api.dsl.Cached.Exclusive;
5353
import com.oracle.truffle.api.dsl.Cached.Shared;
54-
import com.oracle.truffle.api.dsl.CachedLanguage;
5554
import com.oracle.truffle.api.library.CachedLibrary;
5655
import com.oracle.truffle.api.library.ExportLibrary;
5756
import com.oracle.truffle.api.library.ExportMessage;
@@ -85,15 +84,9 @@ public Object getItemWithState(Object key, ThreadState state,
8584

8685
@ExportMessage
8786
public HashingStorage setItemWithState(Object key, Object value, ThreadState state,
88-
@CachedLanguage PythonLanguage lang,
8987
@CachedLibrary(limit = "2") HashingStorageLibrary lib,
9088
@Exclusive @Cached ConditionProfile gotState) {
91-
HashingStorage newStore;
92-
if (key instanceof String) {
93-
newStore = new DynamicObjectStorage(lang);
94-
} else {
95-
newStore = EconomicMapStorage.create();
96-
}
89+
HashingStorage newStore = PDict.createNewStorage(key instanceof String, 1);
9790
if (gotState.profile(state != null)) {
9891
lib.setItemWithState(newStore, key, value, state);
9992
} else {

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

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,8 @@
7272
*/
7373
@ExportLibrary(HashingStorageLibrary.class)
7474
public class HashMapStorage extends HashingStorage {
75+
public static final int SIZE_THRESHOLD = 100;
76+
7577
private final LinkedHashMap<Object, Object> values;
7678

7779
public HashMapStorage(int capacity) {
@@ -224,11 +226,6 @@ static HashingStorage setItem(HashMapStorage self, Object key, Object value, @Su
224226
return self;
225227
}
226228

227-
@TruffleBoundary
228-
private static void put(LinkedHashMap<Object, Object> values, Object key, Object value) {
229-
values.put(key, value);
230-
}
231-
232229
@Specialization(guards = "!isSupportedKey(key, profile)", limit = "3")
233230
static HashingStorage setItemNotSupportedKey(HashMapStorage self, Object key, Object value, @SuppressWarnings("unused") ThreadState state,
234231
@SuppressWarnings("unused") @Cached IsBuiltinClassProfile profile,
@@ -330,4 +327,14 @@ private static Iterator<Object> getReverseIterator(LinkedHashMap<Object, Object>
330327
Collections.reverse(keys);
331328
return keys.iterator();
332329
}
330+
331+
public void put(String key, Object value) {
332+
put(values, key, value);
333+
}
334+
335+
@TruffleBoundary
336+
private static void put(LinkedHashMap<Object, Object> values, Object key, Object value) {
337+
values.put(key, value);
338+
}
339+
333340
}

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@
4242

4343
import static com.oracle.graal.python.runtime.exception.PythonErrorType.TypeError;
4444

45-
import com.oracle.graal.python.PythonLanguage;
4645
import com.oracle.graal.python.builtins.objects.PNone;
4746
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodesFactory.LenNodeGen;
4847
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodesFactory.SetItemNodeGen;
@@ -62,7 +61,6 @@
6261
import com.oracle.graal.python.runtime.exception.PException;
6362
import com.oracle.truffle.api.CompilerDirectives;
6463
import com.oracle.truffle.api.dsl.Cached;
65-
import com.oracle.truffle.api.dsl.CachedLanguage;
6664
import com.oracle.truffle.api.dsl.Fallback;
6765
import com.oracle.truffle.api.dsl.GenerateUncached;
6866
import com.oracle.truffle.api.dsl.ImportStatic;
@@ -180,9 +178,8 @@ static HashingStorage doPDictView(@SuppressWarnings("unused") VirtualFrame frame
180178
@Specialization
181179
static HashingStorage doString(VirtualFrame frame, String str, Object value,
182180
@Cached ConditionProfile hasFrame,
183-
@CachedLanguage PythonLanguage language,
184181
@CachedLibrary(limit = "1") HashingStorageLibrary lib) {
185-
HashingStorage storage = PDict.createNewStorage(language, true, PString.length(str));
182+
HashingStorage storage = PDict.createNewStorage(true, PString.length(str));
186183
Object val = value == PNone.NO_VALUE ? PNone.NONE : value;
187184
for (int i = 0; i < PString.length(str); i++) {
188185
String key = PString.valueOf(PString.charAt(str, i));
@@ -194,9 +191,8 @@ static HashingStorage doString(VirtualFrame frame, String str, Object value,
194191
@Specialization
195192
static HashingStorage doString(VirtualFrame frame, PString pstr, Object value,
196193
@Cached ConditionProfile hasFrame,
197-
@CachedLanguage PythonLanguage language,
198194
@CachedLibrary(limit = "1") HashingStorageLibrary lib) {
199-
return doString(frame, pstr.getValue(), value, hasFrame, language, lib);
195+
return doString(frame, pstr.getValue(), value, hasFrame, lib);
200196
}
201197

202198
@Specialization(guards = {"!isPHashingCollection(other)", "!isDictKeysView(other)", "!isString(other)"}, limit = "getCallSiteInlineCacheMaxDepth()")

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

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -190,16 +190,15 @@ HashingStorage doPDictKwargs(VirtualFrame frame, PHashingCollection iterable, PK
190190
}
191191

192192
@Specialization(guards = "hasIterAttrButNotBuiltin(col, colLib)", limit = "1")
193-
static HashingStorage doNoBuiltinKeysAttr(VirtualFrame frame, PHashingCollection col, @SuppressWarnings("unused") PKeyword[] kwargs,
194-
@CachedLanguage PythonLanguage lang,
193+
HashingStorage doNoBuiltinKeysAttr(VirtualFrame frame, PHashingCollection col, @SuppressWarnings("unused") PKeyword[] kwargs,
195194
@SuppressWarnings("unused") @CachedLibrary("col") PythonObjectLibrary colLib,
196195
@CachedLibrary(limit = "getCallSiteInlineCacheMaxDepth()") PythonObjectLibrary keysLib,
197196
@CachedLibrary(limit = "3") HashingStorageLibrary lib,
198197
@Cached("create(KEYS)") LookupAndCallUnaryNode callKeysNode,
199198
@Cached("create(__GETITEM__)") LookupAndCallBinaryNode callGetItemNode,
200199
@Cached GetNextNode nextNode,
201200
@Cached IsBuiltinClassProfile errorProfile) {
202-
HashingStorage curStorage = PDict.createNewStorage(lang, false, 0);
201+
HashingStorage curStorage = PDict.createNewStorage(false, 0);
203202
return copyToStorage(frame, col, kwargs, curStorage, callKeysNode, callGetItemNode, keysLib, nextNode, errorProfile, lib);
204203
}
205204

@@ -209,21 +208,19 @@ protected static boolean hasIterAttrButNotBuiltin(PHashingCollection col, Python
209208
}
210209

211210
@Specialization(guards = {"!isPDict(mapping)", "hasKeysAttribute(mapping)"})
212-
static HashingStorage doMapping(VirtualFrame frame, Object mapping, PKeyword[] kwargs,
213-
@CachedLanguage PythonLanguage lang,
211+
HashingStorage doMapping(VirtualFrame frame, Object mapping, PKeyword[] kwargs,
214212
@CachedLibrary(limit = "3") HashingStorageLibrary lib,
215213
@CachedLibrary(limit = "getCallSiteInlineCacheMaxDepth()") PythonObjectLibrary keysLib,
216214
@Cached("create(KEYS)") LookupAndCallUnaryNode callKeysNode,
217215
@Cached("create(__GETITEM__)") LookupAndCallBinaryNode callGetItemNode,
218216
@Cached GetNextNode nextNode,
219217
@Cached IsBuiltinClassProfile errorProfile) {
220-
HashingStorage curStorage = PDict.createNewStorage(lang, false, 0);
218+
HashingStorage curStorage = PDict.createNewStorage(false, 0);
221219
return copyToStorage(frame, mapping, kwargs, curStorage, callKeysNode, callGetItemNode, keysLib, nextNode, errorProfile, lib);
222220
}
223221

224222
@Specialization(guards = {"!isNoValue(iterable)", "!isPDict(iterable)", "!hasKeysAttribute(iterable)"}, limit = "getCallSiteInlineCacheMaxDepth()")
225-
static HashingStorage doSequence(VirtualFrame frame, PythonObject iterable, PKeyword[] kwargs,
226-
@CachedLanguage PythonLanguage lang,
223+
HashingStorage doSequence(VirtualFrame frame, PythonObject iterable, PKeyword[] kwargs,
227224
@CachedLibrary(limit = "3") HashingStorageLibrary lib,
228225
@CachedLibrary("iterable") PythonObjectLibrary iterableLib,
229226
@Cached PRaiseNode raise,
@@ -235,7 +232,7 @@ static HashingStorage doSequence(VirtualFrame frame, PythonObject iterable, PKey
235232
@Cached IsBuiltinClassProfile errorProfile,
236233
@Cached IsBuiltinClassProfile isTypeErrorProfile) {
237234

238-
return addSequenceToStorage(frame, iterable, kwargs, (isStringKey, expectedSize) -> PDict.createNewStorage(lang, isStringKey, expectedSize), iterableLib, nextNode, createListNode,
235+
return addSequenceToStorage(frame, iterable, kwargs, (isStringKey, expectedSize) -> PDict.createNewStorage(isStringKey, expectedSize), iterableLib, nextNode, createListNode,
239236
seqLenNode, lengthTwoProfile, raise, getItemNode, isTypeErrorProfile,
240237
errorProfile, lib);
241238
}

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

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@
4343
import java.util.Iterator;
4444
import java.util.NoSuchElementException;
4545

46-
import com.oracle.graal.python.PythonLanguage;
4746
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary.ForEachNode;
4847
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary.HashingStorageIterable;
4948
import com.oracle.graal.python.builtins.objects.dict.PDict;
@@ -55,7 +54,6 @@
5554
import com.oracle.graal.python.nodes.object.IsBuiltinClassProfile;
5655
import com.oracle.truffle.api.dsl.Cached;
5756
import com.oracle.truffle.api.dsl.Cached.Exclusive;
58-
import com.oracle.truffle.api.dsl.CachedLanguage;
5957
import com.oracle.truffle.api.dsl.ImportStatic;
6058
import com.oracle.truffle.api.dsl.Specialization;
6159
import com.oracle.truffle.api.library.CachedLibrary;
@@ -183,9 +181,8 @@ static Object notString(KeywordsStorage self, Object key, ThreadState state,
183181
@ExportMessage
184182
public HashingStorage setItemWithState(Object key, Object value, ThreadState state,
185183
@CachedLibrary(limit = "2") HashingStorageLibrary lib,
186-
@CachedLanguage PythonLanguage language,
187184
@Exclusive @Cached ConditionProfile gotState) {
188-
HashingStorage newStore = generalize(lib, language, key instanceof String, length() + 1);
185+
HashingStorage newStore = generalize(lib, key instanceof String, length() + 1);
189186
if (gotState.profile(state != null)) {
190187
return lib.setItemWithState(newStore, key, value, state);
191188
} else {
@@ -196,18 +193,17 @@ public HashingStorage setItemWithState(Object key, Object value, ThreadState sta
196193
@ExportMessage
197194
public HashingStorage delItemWithState(Object key, ThreadState state,
198195
@CachedLibrary(limit = "1") HashingStorageLibrary lib,
199-
@CachedLanguage PythonLanguage language,
200196
@Exclusive @Cached ConditionProfile gotState) {
201-
HashingStorage newStore = generalize(lib, language, true, length() - 1);
197+
HashingStorage newStore = generalize(lib, true, length() - 1);
202198
if (gotState.profile(state != null)) {
203199
return lib.delItemWithState(newStore, key, state);
204200
} else {
205201
return lib.delItem(newStore, key);
206202
}
207203
}
208204

209-
private HashingStorage generalize(HashingStorageLibrary lib, PythonLanguage language, boolean isStringKey, int expectedLength) {
210-
HashingStorage newStore = PDict.createNewStorage(language, isStringKey, expectedLength);
205+
private HashingStorage generalize(HashingStorageLibrary lib, boolean isStringKey, int expectedLength) {
206+
HashingStorage newStore = PDict.createNewStorage(isStringKey, expectedLength);
211207
newStore = lib.addAllToOther(this, newStore);
212208
return newStore;
213209
}

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

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646
import java.util.List;
4747
import java.util.NoSuchElementException;
4848

49-
import com.oracle.graal.python.PythonLanguage;
5049
import com.oracle.graal.python.builtins.objects.cell.PCell;
5150
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary.ForEachNode;
5251
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary.HashingStorageIterable;
@@ -62,7 +61,6 @@
6261
import com.oracle.truffle.api.Truffle;
6362
import com.oracle.truffle.api.dsl.Cached;
6463
import com.oracle.truffle.api.dsl.Cached.Exclusive;
65-
import com.oracle.truffle.api.dsl.CachedLanguage;
6664
import com.oracle.truffle.api.dsl.ImportStatic;
6765
import com.oracle.truffle.api.dsl.Specialization;
6866
import com.oracle.truffle.api.frame.FrameDescriptor;
@@ -191,9 +189,8 @@ private static FrameSlot findSlot(LocalsStorage self, Object key) {
191189
@ExportMessage
192190
HashingStorage setItemWithState(Object key, Object value, ThreadState state,
193191
@CachedLibrary(limit = "2") HashingStorageLibrary lib,
194-
@CachedLanguage PythonLanguage language,
195192
@Exclusive @Cached ConditionProfile gotState) {
196-
HashingStorage result = generalize(lib, language, key instanceof String, length() + 1);
193+
HashingStorage result = generalize(lib, key instanceof String, length() + 1);
197194
if (gotState.profile(state != null)) {
198195
return lib.setItemWithState(result, key, value, state);
199196
} else {
@@ -204,18 +201,17 @@ HashingStorage setItemWithState(Object key, Object value, ThreadState state,
204201
@ExportMessage
205202
HashingStorage delItemWithState(Object key, ThreadState state,
206203
@CachedLibrary(limit = "1") HashingStorageLibrary lib,
207-
@CachedLanguage PythonLanguage language,
208204
@Exclusive @Cached ConditionProfile gotState) {
209-
HashingStorage result = generalize(lib, language, true, length() - 1);
205+
HashingStorage result = generalize(lib, true, length() - 1);
210206
if (gotState.profile(state != null)) {
211207
return lib.delItemWithState(result, key, state);
212208
} else {
213209
return lib.delItem(result, key);
214210
}
215211
}
216212

217-
private HashingStorage generalize(HashingStorageLibrary lib, PythonLanguage language, boolean isStringKey, int expectedLength) {
218-
HashingStorage newStore = PDict.createNewStorage(language, isStringKey, expectedLength);
213+
private HashingStorage generalize(HashingStorageLibrary lib, boolean isStringKey, int expectedLength) {
214+
HashingStorage newStore = PDict.createNewStorage(isStringKey, expectedLength);
219215
newStore = lib.addAllToOther(this, newStore);
220216
return newStore;
221217
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/dict/PDict.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131

3232
import com.oracle.graal.python.PythonLanguage;
3333
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
34-
import com.oracle.graal.python.builtins.objects.common.DynamicObjectStorage;
3534
import com.oracle.graal.python.builtins.objects.common.EconomicMapStorage;
3635
import com.oracle.graal.python.builtins.objects.common.EmptyStorage;
3736
import com.oracle.graal.python.builtins.objects.common.HashMapStorage;
@@ -84,16 +83,12 @@ public void setItem(Object key, Object value) {
8483
storage = HashingStorageLibrary.getUncached().setItem(storage, key, value);
8584
}
8685

87-
public static HashingStorage createNewStorage(PythonLanguage lang, boolean isStringKey, int expectedSize) {
86+
public static HashingStorage createNewStorage(boolean isStringKey, int expectedSize) {
8887
HashingStorage newDictStorage;
8988
if (expectedSize == 0) {
9089
newDictStorage = EmptyStorage.INSTANCE;
9190
} else if (isStringKey) {
92-
if (expectedSize < DynamicObjectStorage.SIZE_THRESHOLD) {
93-
newDictStorage = new DynamicObjectStorage(lang);
94-
} else {
95-
newDictStorage = new HashMapStorage(expectedSize);
96-
}
91+
newDictStorage = new HashMapStorage(expectedSize);
9792
} else {
9893
newDictStorage = EconomicMapStorage.create(expectedSize);
9994
}

0 commit comments

Comments
 (0)