Skip to content

Commit 72aff88

Browse files
committed
Rename GetHashingStorageNode to GetSetStorageNode and review usages
* GetHashingStorageNode, as documented, makes no guarantee about having values. This means it should not be used in any place which might requires values, and so only be used for set & frozenset usages. * Review all usages and replace them with appropriate more direct access to the storage instead.
1 parent 67000c6 commit 72aff88

File tree

12 files changed

+166
-178
lines changed

12 files changed

+166
-178
lines changed

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/functools/PartialBuiltins.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@
6565
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
6666
import com.oracle.graal.python.builtins.PythonBuiltins;
6767
import com.oracle.graal.python.builtins.objects.PNone;
68-
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes;
6968
import com.oracle.graal.python.builtins.objects.common.HashingStorage;
7069
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodes.HashingStorageAddAllToOther;
7170
import com.oracle.graal.python.builtins.objects.common.HashingStorageNodes.HashingStorageCopy;
@@ -352,7 +351,6 @@ static Object setState(VirtualFrame frame, PPartial self, PTuple state,
352351
@Cached PyDictCheckExactNode dictCheckExactNode,
353352
@Cached PyTupleGetItem getItemNode,
354353
@Cached TupleNodes.ConstructTupleNode constructTupleNode,
355-
@Cached HashingCollectionNodes.GetHashingStorageNode getHashingStorageNode,
356354
@Cached HashingStorageCopy copyStorageNode,
357355
@Cached PythonObjectFactory factory,
358356
@Cached PRaiseNode.Lazy raiseNode) {
@@ -385,7 +383,7 @@ static Object setState(VirtualFrame frame, PPartial self, PTuple state,
385383
if (fnKwargs == PNone.NONE) {
386384
fnKwargsDict = factory.createDict();
387385
} else if (!dictCheckExactNode.execute(inliningTarget, fnKwargs)) {
388-
fnKwargsDict = factory.createDict(copyStorageNode.execute(inliningTarget, getHashingStorageNode.execute(frame, inliningTarget, fnKwargs)));
386+
fnKwargsDict = factory.createDict(copyStorageNode.execute(inliningTarget, ((PDict) fnKwargs).getDictStorage()));
389387
} else {
390388
fnKwargsDict = (PDict) fnKwargs;
391389
}

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/pickle/PPickler.java

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@
6363
import java.util.Map;
6464
import java.util.WeakHashMap;
6565

66+
import com.oracle.graal.python.builtins.objects.set.PFrozenSet;
67+
import com.oracle.graal.python.builtins.objects.set.PSet;
6668
import org.graalvm.collections.Pair;
6769

6870
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
@@ -1413,7 +1415,7 @@ private void saveUnicode(VirtualFrame frame, PPickler pickler, Object obj) {
14131415
}
14141416

14151417
private void batchDictExact(VirtualFrame frame, PPickler pickler, PDict dict) {
1416-
final HashingStorage storage = getHashingStorage(frame, dict);
1418+
final HashingStorage storage = dict.getDictStorage();
14171419
final int length = getHashingStorageLength(storage);
14181420
// Special-case len(d) == 1 to save space.
14191421
if (length == 1) {
@@ -1478,9 +1480,8 @@ private void saveDict(VirtualFrame frame, Node inliningTarget, PyObjectCallMetho
14781480
}
14791481
}
14801482

1481-
private void batchSetExact(VirtualFrame frame, PPickler pickler, Object obj) {
1482-
final HashingStorage storage = getHashingStorage(frame, obj);
1483-
saveSetHashingStorageBatched(frame, pickler, storage);
1483+
private void batchSetExact(VirtualFrame frame, PPickler pickler, PSet set) {
1484+
saveSetHashingStorageBatched(frame, pickler, set.getDictStorage());
14841485
}
14851486

14861487
private void batchSet(VirtualFrame frame, PPickler pickler, Object obj) {
@@ -1526,8 +1527,8 @@ private void saveSet(VirtualFrame frame, PythonContext ctx, PPickler pickler, Ob
15261527
}
15271528

15281529
// Write in batches of BATCHSIZE.
1529-
if (PGuards.isPSet(obj)) {
1530-
batchSetExact(frame, pickler, obj);
1530+
if (obj instanceof PSet set) {
1531+
batchSetExact(frame, pickler, set);
15311532
} else {
15321533
batchSet(frame, pickler, obj);
15331534
}
@@ -1547,9 +1548,8 @@ private void saveFrozenset(VirtualFrame frame, PythonContext ctx, PPickler pickl
15471548

15481549
write(pickler, PickleUtils.OPCODE_MARK);
15491550

1550-
if (PGuards.isPFrozenSet(obj)) {
1551-
final HashingStorage storage = getHashingStorage(frame, obj);
1552-
saveSetHashingStorage(frame, pickler, storage);
1551+
if (obj instanceof PFrozenSet set) {
1552+
saveSetHashingStorage(frame, pickler, set.getDictStorage());
15531553
} else {
15541554
final Object iterator = getIter(frame, obj);
15551555
saveFrozenSetIterator(frame, pickler, iterator);

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/pickle/PUnpickler.java

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -736,7 +736,7 @@ protected Object pDataPop(PUnpickler self) {
736736
return pDataPopNode.execute(self.stack);
737737
}
738738

739-
protected Object pDataPopTuple(PUnpickler self, int start) {
739+
protected PTuple pDataPopTuple(PUnpickler self, int start) {
740740
if (pDataPopTupleNode == null) {
741741
CompilerDirectives.transferToInterpreterAndInvalidate();
742742
pDataPopTupleNode = insert(PData.PDataPopTupleNode.create());
@@ -1145,8 +1145,8 @@ private void loadAddItems(VirtualFrame frame, PUnpickler self) {
11451145

11461146
Object set = self.stack.data[mark - 1];
11471147
if (set instanceof PSet) {
1148-
Object items = pDataPopTuple(self, mark);
1149-
final HashingStorage union = ((PSet) set).getDictStorage().unionCached(getHashingStorage(frame, items), ensureHashingStorageCopy(), ensureHashingStorageAddAllToOther());
1148+
PTuple items = pDataPopTuple(self, mark);
1149+
final HashingStorage union = ((PSet) set).getDictStorage().unionCached(getClonedHashingStorage(frame, items), ensureHashingStorageCopy(), ensureHashingStorageAddAllToOther());
11501150
((PSet) set).setDictStorage(union);
11511151
} else {
11521152
Object add_func;
@@ -1413,10 +1413,8 @@ private void loadBuild(VirtualFrame frame, PUnpickler self) {
14131413
}
14141414
Object dict = lookupAttributeStrict(frame, inst, T___DICT__);
14151415

1416-
final HashingStorage storage = getHashingStorage(frame, state);
1416+
final HashingStorage storage = ((PDict) state).getDictStorage();
14171417
// entries = hashLib.entries(storage);
1418-
final HashingStorage dictStorage = getHashingStorage(frame, dict);
1419-
ensureHashingStorageAddAllToOther().executeCached(frame, storage, dictStorage);
14201418

14211419
HashingStorageIterator it = getHashingStorageIterator(storage);
14221420
HashingStorageIteratorNext nextNode = ensureHashingStorageIteratorNext();
@@ -1429,9 +1427,7 @@ private void loadBuild(VirtualFrame frame, PUnpickler self) {
14291427
// the keys)
14301428
// if (PyUnicode_CheckExact(d_key))
14311429
// PyUnicode_InternInPlace(&d_key);
1432-
// TODO: shouldn't this be setting the storage back to the dict? The issue is
1433-
// that it may be a temporary storage created in getHashingStorage
1434-
setHashingStorageItem(frame, dictStorage, getKeyNode.executeCached(storage, it), getValueNode.executeCached(storage, it));
1430+
pyObjectSetItem(frame, dict, getKeyNode.executeCached(storage, it), getValueNode.executeCached(storage, it));
14351431
}
14361432
}
14371433

@@ -1440,7 +1436,7 @@ private void loadBuild(VirtualFrame frame, PUnpickler self) {
14401436
if (!(slotstate instanceof PDict)) {
14411437
throw raise(PythonBuiltinClassType.UnpicklingError, ErrorMessages.S_STATE_NOT_DICT, "slot");
14421438
}
1443-
final HashingStorage storage = getHashingStorage(frame, slotstate);
1439+
final HashingStorage storage = ((PDict) slotstate).getDictStorage();
14441440
HashingStorageIterator it = getHashingStorageIterator(storage);
14451441
HashingStorageIteratorNext nextNode = ensureHashingStorageIteratorNext();
14461442
HashingStorageIteratorKey getKeyNode = ensureHashingStorageIteratorKey();
@@ -1766,9 +1762,7 @@ private void loadExtension(VirtualFrame frame, PythonContext ctx, PUnpickler sel
17661762
obj = findClass(frame, ctx.getCore(), self, moduleName, className);
17671763

17681764
// Cache code -> obj.
1769-
// cpython expects a PyDict so this is a safe cast
1770-
assert st.extensionCache instanceof PDict;
1771-
setDictItem(frame, (PDict) st.extensionCache, code, obj);
1765+
setDictItem(frame, st.extensionCache, code, obj);
17721766
pDataPush(self, obj);
17731767
}
17741768

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/pickle/PickleState.java

Lines changed: 56 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,11 @@
4444
import static com.oracle.graal.python.util.PythonUtils.tsLiteral;
4545

4646
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
47+
import com.oracle.graal.python.builtins.objects.dict.PDict;
4748
import com.oracle.graal.python.builtins.objects.module.PythonModule;
4849
import com.oracle.graal.python.lib.PyCallableCheckNode;
4950
import com.oracle.graal.python.lib.PyObjectGetAttr;
5051
import com.oracle.graal.python.nodes.ErrorMessages;
51-
import com.oracle.graal.python.nodes.PGuards;
5252
import com.oracle.graal.python.nodes.PRaiseNode;
5353
import com.oracle.graal.python.runtime.PythonContext;
5454
import com.oracle.truffle.api.dsl.Bind;
@@ -61,15 +61,15 @@
6161

6262
public class PickleState {
6363
// copyreg.dispatch_table, {type_object: pickling_function}
64-
Object dispatchTable;
64+
PDict dispatchTable;
6565

6666
// For the extension opcodes EXT1, EXT2 and EXT4.
6767
// copyreg._extension_registry, {(module_name, function_name): code}
68-
Object extensionRegistry;
68+
PDict extensionRegistry;
6969
// copyreg._extension_cache, {code: object}
70-
Object extensionCache;
70+
PDict extensionCache;
7171
// copyreg._inverted_registry, {code: (module_name, function_name)}
72-
Object invertedRegistry;
72+
PDict invertedRegistry;
7373

7474
// codecs.encode, used for saving bytes in older protocols
7575
Object codecsEncode;
@@ -81,12 +81,12 @@ public class PickleState {
8181

8282
// Import mappings for compatibility with Python 2.x
8383
// * _compat_pickle.NAME_MAPPING, {(oldmodule, oldname): (newmodule, newname)}
84-
Object nameMapping2To3;
84+
PDict nameMapping2To3;
8585
// _compat_pickle.IMPORT_MAPPING, {oldmodule: newmodule}
86-
Object importMapping2To3;
86+
PDict importMapping2To3;
8787
// Same, but with REVERSE_NAME_MAPPING / REVERSE_IMPORT_MAPPING
88-
Object nameMapping3To2;
89-
Object importMapping3To2;
88+
PDict nameMapping3To2;
89+
PDict importMapping3To2;
9090

9191
@GenerateUncached
9292
@GenerateInline(false) // footprint reduction 36 -> 18
@@ -106,57 +106,75 @@ void init(PickleState state,
106106
final PythonModule builtins = context.getBuiltins();
107107
state.getattr = getAttr.execute(null, inliningTarget, builtins, T_GETATTR);
108108

109-
final Object copyreg = importModule(PickleUtils.T_MOD_COPYREG);
110-
state.dispatchTable = getAttr.execute(null, inliningTarget, copyreg, PickleUtils.T_ATTR_DISPATCH_TABLE);
111-
if (!PGuards.isDict(state.dispatchTable)) {
112-
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "copyreg.dispatch_table", "dict", state.dispatchTable);
109+
var copyreg = importModule(PickleUtils.T_MOD_COPYREG);
110+
var dispatchTable = getAttr.execute(null, inliningTarget, copyreg, PickleUtils.T_ATTR_DISPATCH_TABLE);
111+
if (dispatchTable instanceof PDict dispatchTableDict) {
112+
state.dispatchTable = dispatchTableDict;
113+
} else {
114+
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "copyreg.dispatch_table", "dict", dispatchTable);
113115
}
114116

115-
state.extensionRegistry = getAttr.execute(null, inliningTarget, copyreg, PickleUtils.T_ATTR_EXT_REGISTRY);
116-
if (!PGuards.isDict(state.extensionRegistry)) {
117-
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "copyreg._extension_registry", "dict", state.extensionRegistry);
117+
var extensionRegistry = getAttr.execute(null, inliningTarget, copyreg, PickleUtils.T_ATTR_EXT_REGISTRY);
118+
if (extensionRegistry instanceof PDict extensionRegistryDict) {
119+
state.extensionRegistry = extensionRegistryDict;
120+
} else {
121+
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "copyreg._extension_registry", "dict", extensionRegistry);
118122
}
119123

120-
state.invertedRegistry = getAttr.execute(null, inliningTarget, copyreg, PickleUtils.T_ATTR_INV_REGISTRY);
121-
if (!PGuards.isDict(state.invertedRegistry)) {
122-
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "copyreg._inverted_registry", "dict", state.invertedRegistry);
124+
var invertedRegistry = getAttr.execute(null, inliningTarget, copyreg, PickleUtils.T_ATTR_INV_REGISTRY);
125+
if (invertedRegistry instanceof PDict invertedRegistryDict) {
126+
state.invertedRegistry = invertedRegistryDict;
127+
} else {
128+
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "copyreg._inverted_registry", "dict", invertedRegistry);
123129
}
124130

125-
state.extensionCache = getAttr.execute(null, inliningTarget, copyreg, PickleUtils.T_ATTR_EXT_CACHE);
126-
if (!PGuards.isDict(state.extensionCache)) {
127-
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "copyreg._extension_cache", "dict", state.extensionCache);
131+
var extensionCache = getAttr.execute(null, inliningTarget, copyreg, PickleUtils.T_ATTR_EXT_CACHE);
132+
if (extensionCache instanceof PDict extensionCacheDict) {
133+
state.extensionCache = extensionCacheDict;
134+
} else {
135+
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "copyreg._extension_cache", "dict", extensionCache);
128136
}
129137

130138
final Object codecs = importModule(PickleUtils.T_MOD_CODECS);
131-
state.codecsEncode = getAttr.execute(null, inliningTarget, codecs, PickleUtils.T_METHOD_ENCODE);
132-
if (!callableCheck.execute(inliningTarget, state.codecsEncode)) {
133-
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "codecs.encode", "callable", state.codecsEncode);
139+
var codecsEncode = getAttr.execute(null, inliningTarget, codecs, PickleUtils.T_METHOD_ENCODE);
140+
if (callableCheck.execute(inliningTarget, codecsEncode)) {
141+
state.codecsEncode = codecsEncode;
142+
} else {
143+
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, "codecs.encode", "callable", codecsEncode);
134144
}
135145

136146
final Object functools = importModule(PickleUtils.T_MOD_FUNCTOOLS);
137147
state.partial = getAttr.execute(null, inliningTarget, functools, PickleUtils.T_METHOD_PARTIAL);
138148

139149
// Load the 2.x -> 3.x stdlib module mapping tables
140150
Object compatPickle = importModule(PickleUtils.T_MOD_COMPAT_PICKLE);
141-
state.nameMapping2To3 = getAttr.execute(null, inliningTarget, compatPickle, PickleUtils.T_ATTR_NAME_MAPPING);
142-
if (!PGuards.isDict(state.nameMapping2To3)) {
143-
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, PickleUtils.T_CP_NAME_MAPPING, "dict", state.nameMapping2To3);
151+
var nameMapping2To3 = getAttr.execute(null, inliningTarget, compatPickle, PickleUtils.T_ATTR_NAME_MAPPING);
152+
if (nameMapping2To3 instanceof PDict nameMapping2To3Dict) {
153+
state.nameMapping2To3 = nameMapping2To3Dict;
154+
} else {
155+
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, PickleUtils.T_CP_NAME_MAPPING, "dict", nameMapping2To3);
144156
}
145-
state.importMapping2To3 = getAttr.execute(null, inliningTarget, compatPickle, PickleUtils.T_ATTR_IMPORT_MAPPING);
146-
if (!PGuards.isDict(state.nameMapping2To3)) {
147-
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, PickleUtils.T_CP_IMPORT_MAPPING, "dict", state.importMapping2To3);
157+
158+
var importMapping2To3 = getAttr.execute(null, inliningTarget, compatPickle, PickleUtils.T_ATTR_IMPORT_MAPPING);
159+
if (importMapping2To3 instanceof PDict importMapping2To3Dict) {
160+
state.importMapping2To3 = importMapping2To3Dict;
161+
} else {
162+
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, PickleUtils.T_CP_IMPORT_MAPPING, "dict", importMapping2To3);
148163
}
149164

150165
// ... and the 3.x -> 2.x mapping tables
151-
state.nameMapping3To2 = getAttr.execute(null, inliningTarget, compatPickle, PickleUtils.T_ATTR_REVERSE_NAME_MAPPING);
152-
if (!PGuards.isDict(state.nameMapping2To3)) {
153-
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, PickleUtils.T_CP_REVERSE_NAME_MAPPING, "dict",
154-
state.nameMapping3To2);
166+
var nameMapping3To2 = getAttr.execute(null, inliningTarget, compatPickle, PickleUtils.T_ATTR_REVERSE_NAME_MAPPING);
167+
if (nameMapping3To2 instanceof PDict nameMapping3To2Dict) {
168+
state.nameMapping3To2 = nameMapping3To2Dict;
169+
} else {
170+
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, PickleUtils.T_CP_REVERSE_NAME_MAPPING, "dict", nameMapping3To2);
155171
}
156-
state.importMapping3To2 = getAttr.execute(null, inliningTarget, compatPickle, PickleUtils.T_ATTR_REVERSE_IMPORT_MAPPING);
157-
if (!PGuards.isDict(state.nameMapping2To3)) {
172+
var importMapping3To2 = getAttr.execute(null, inliningTarget, compatPickle, PickleUtils.T_ATTR_REVERSE_IMPORT_MAPPING);
173+
if (importMapping3To2 instanceof PDict importMapping3To2Dict) {
174+
state.importMapping3To2 = importMapping3To2Dict;
175+
} else {
158176
throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.RuntimeError, ErrorMessages.S_SHOULD_BE_A_S_NOT_A_P, PickleUtils.T_CP_REVERSE_IMPORT_MAPPING, "dict",
159-
state.importMapping3To2);
177+
importMapping3To2);
160178
}
161179
}
162180
}

0 commit comments

Comments
 (0)