Skip to content

Commit 66a1db5

Browse files
steve-ssteve-s
committed
[GR-40610] Fix obj hashmap side effects.
PullRequest: graalpython/2410
2 parents 94771ca + 652ba03 commit 66a1db5

File tree

4 files changed

+39
-22
lines changed

4 files changed

+39
-22
lines changed

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

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@
7171
import com.oracle.truffle.api.library.ExportLibrary;
7272
import com.oracle.truffle.api.library.ExportMessage;
7373
import com.oracle.truffle.api.nodes.LoopNode;
74+
import com.oracle.truffle.api.profiles.BranchProfile;
7475
import com.oracle.truffle.api.profiles.ConditionProfile;
7576
import com.oracle.truffle.api.profiles.LoopConditionProfile;
7677
import com.oracle.truffle.api.strings.TruffleString;
@@ -219,6 +220,7 @@ static HashingStorage setItemGeneric(EconomicMapStorage self, Object key, Object
219220
@Shared("putNode") @Cached ObjectHashMap.PutNode putNode,
220221
@Shared("hashNode") @Cached PyObjectHashNode hashNode,
221222
@Shared("gotState") @Cached ConditionProfile gotState) {
223+
convertToSideEffectMap(self);
222224
VirtualFrame frame = gotState.profile(state == null) ? null : PArguments.frameForCall(state);
223225
putNode.put(state, self.map, key, hashNode.execute(frame, key), assertNoJavaString(value));
224226
return self;
@@ -401,14 +403,12 @@ static int compareSameType(EconomicMapStorage self, EconomicMapStorage other, Th
401403
@CachedLibrary("self") HashingStorageLibrary thisLib,
402404
@Shared("selfEntriesLoop") @Cached LoopConditionProfile loopProfile,
403405
@Shared("selfEntriesLoopExit") @Cached LoopConditionProfile earlyExitProfile,
404-
@Shared("getNode") @Cached ObjectHashMap.GetNode getNode,
405-
@Shared("gotState") @Cached ConditionProfile gotState) {
406+
@Shared("getNode") @Cached ObjectHashMap.GetNode getNode) {
406407
int size = self.map.size();
407408
int size2 = other.map.size();
408409
if (size > size2) {
409410
return 1;
410411
}
411-
VirtualFrame frame = gotState.profile(state == null) ? null : PArguments.frameForCall(state);
412412
MapCursor cursor = self.map.getEntries();
413413
int counter = 0;
414414
try {
@@ -477,17 +477,22 @@ static HashingStorage intersectSameType(EconomicMapStorage self, EconomicMapStor
477477
@Shared("putNode") @Cached ObjectHashMap.PutNode putNode,
478478
@Shared("getNode") @Cached ObjectHashMap.GetNode getNode,
479479
@Shared("selfEntriesLoop") @Cached LoopConditionProfile loopProfile,
480-
@Shared("gotState") @Cached ConditionProfile gotState) {
480+
@Shared("setSideEffect") @Cached BranchProfile setSideEffectFlag) {
481481
EconomicMapStorage result = EconomicMapStorage.create();
482-
VirtualFrame frame = gotState.profile(state == null) ? null : PArguments.frameForCall(state);
482+
ObjectHashMap resultMap = result.map;
483+
ObjectHashMap otherMap = other.map;
484+
if (self.map.hasSideEffect() || otherMap.hasSideEffect()) {
485+
setSideEffectFlag.enter();
486+
resultMap.setSideEffectingKeysFlag();
487+
}
483488
MapCursor cursor = self.map.getEntries();
484489
// get/put may throw, but we ignore that small inaccuracy
485490
final int size = self.map.size();
486491
loopProfile.profileCounted(size);
487492
LoopNode.reportLoopCount(thisLib, size);
488493
while (loopProfile.inject(advance(cursor))) {
489-
if (getNode.get(state, other.map, getDictKey(cursor)) != null) {
490-
putNode.put(state, result.map, getDictKey(cursor), getValue(cursor));
494+
if (getNode.get(state, otherMap, getDictKey(cursor)) != null) {
495+
putNode.put(state, resultMap, getDictKey(cursor), getValue(cursor));
491496
}
492497
}
493498
return result;
@@ -498,18 +503,18 @@ static HashingStorage intersectGeneric(EconomicMapStorage self, HashingStorage o
498503
@CachedLibrary("self") HashingStorageLibrary thisLib,
499504
@Shared("putNode") @Cached ObjectHashMap.PutNode putNode,
500505
@Shared("selfEntriesLoop") @Cached LoopConditionProfile loopProfile,
501-
@Shared("otherHLib") @CachedLibrary(limit = "2") HashingStorageLibrary hlib,
502-
@Shared("gotState") @Cached ConditionProfile gotState) {
506+
@Shared("otherHLib") @CachedLibrary(limit = "2") HashingStorageLibrary hlib) {
503507
EconomicMapStorage result = EconomicMapStorage.create();
504-
VirtualFrame frame = gotState.profile(state == null) ? null : PArguments.frameForCall(state);
508+
ObjectHashMap resultMap = result.map;
509+
resultMap.setSideEffectingKeysFlag();
505510
MapCursor cursor = self.map.getEntries();
506511
// get/put may throw, but we ignore that small inaccuracy
507512
final int size = self.map.size();
508513
loopProfile.profileCounted(size);
509514
LoopNode.reportLoopCount(thisLib, size);
510515
while (loopProfile.inject(advance(cursor))) {
511516
if (hlib.hasKey(other, getKey(cursor))) {
512-
putNode.put(state, result.map, getDictKey(cursor), getValue(cursor));
517+
putNode.put(state, resultMap, getDictKey(cursor), getValue(cursor));
513518
}
514519
}
515520
return result;
@@ -524,17 +529,22 @@ static HashingStorage diffSameType(EconomicMapStorage self, EconomicMapStorage o
524529
@Shared("putNode") @Cached ObjectHashMap.PutNode putNode,
525530
@Shared("getNode") @Cached ObjectHashMap.GetNode getNode,
526531
@Shared("selfEntriesLoop") @Cached LoopConditionProfile loopProfile,
527-
@Shared("gotState") @Cached ConditionProfile gotState) {
532+
@Shared("setSideEffect") @Cached BranchProfile setSideEffectFlag) {
528533
EconomicMapStorage result = EconomicMapStorage.create();
529-
VirtualFrame frame = gotState.profile(state == null) ? null : PArguments.frameForCall(state);
534+
ObjectHashMap resultMap = result.map;
535+
ObjectHashMap otherMap = other.map;
536+
if (self.map.hasSideEffect() || otherMap.hasSideEffect()) {
537+
setSideEffectFlag.enter();
538+
resultMap.setSideEffectingKeysFlag();
539+
}
530540
MapCursor cursor = self.map.getEntries();
531541
// get/put may throw, but we ignore that small inaccuracy
532542
final int size = self.map.size();
533543
loopProfile.profileCounted(size);
534544
LoopNode.reportLoopCount(thisLib, size);
535545
while (loopProfile.inject(advance(cursor))) {
536-
if (getNode.get(state, other.map, getDictKey(cursor)) == null) {
537-
putNode.put(state, result.map, getDictKey(cursor), getValue(cursor));
546+
if (getNode.get(state, otherMap, getDictKey(cursor)) == null) {
547+
putNode.put(state, resultMap, getDictKey(cursor), getValue(cursor));
538548
}
539549
}
540550
return result;
@@ -545,18 +555,18 @@ static HashingStorage diffGeneric(EconomicMapStorage self, HashingStorage other,
545555
@CachedLibrary("self") HashingStorageLibrary thisLib,
546556
@Shared("putNode") @Cached ObjectHashMap.PutNode putNode,
547557
@Shared("selfEntriesLoop") @Cached LoopConditionProfile loopProfile,
548-
@Shared("otherHLib") @CachedLibrary(limit = "2") HashingStorageLibrary hlib,
549-
@Shared("gotState") @Cached ConditionProfile gotState) {
558+
@Shared("otherHLib") @CachedLibrary(limit = "2") HashingStorageLibrary hlib) {
550559
EconomicMapStorage result = EconomicMapStorage.create();
551-
VirtualFrame frame = gotState.profile(state == null) ? null : PArguments.frameForCall(state);
560+
ObjectHashMap resultMap = result.map;
561+
resultMap.setSideEffectingKeysFlag();
552562
MapCursor cursor = self.map.getEntries();
553563
// get/put may throw, but we ignore that small inaccuracy
554564
final int size = self.map.size();
555565
loopProfile.profileCounted(size);
556566
LoopNode.reportLoopCount(thisLib, size);
557567
while (loopProfile.profile(advance(cursor))) {
558568
if (!hlib.hasKey(other, getKey(cursor))) {
559-
putNode.put(state, result.map, getDictKey(cursor), getValue(cursor));
569+
putNode.put(state, resultMap, getDictKey(cursor), getValue(cursor));
560570
}
561571
}
562572
return result;

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -694,8 +694,15 @@ public static void doRemove(ThreadState state, ObjectHashMap map, Object key, lo
694694
}
695695

696696
private boolean keysEqual(ThreadState state, int index, Object key, long keyHash, PyObjectRichCompareBool.EqNode eqNode, ConditionProfile hasState) {
697+
if (keyHash != hashes[index]) {
698+
return false;
699+
}
700+
Object originalKey = getKey(index);
701+
if (originalKey == key) {
702+
return true;
703+
}
697704
VirtualFrame frame = hasState.profile(state == null) ? null : PArguments.frameForCall(state);
698-
return hashes[index] == keyHash && eqNode.execute(frame, getKey(index), key);
705+
return eqNode.execute(frame, originalKey, key);
699706
}
700707

701708
/**

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/function/GeneratorExpressionNode.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
public final class GeneratorExpressionNode extends ExpressionDefinitionNode {
4646
private final TruffleString name;
4747
private final TruffleString qualname;
48-
@CompilationFinal(dimensions = 1) private RootCallTarget[] callTargets;
48+
@CompilationFinal(dimensions = 1) private volatile RootCallTarget[] callTargets;
4949
private final FrameDescriptor frameDescriptor;
5050
private final GeneratorInfo generatorInfo;
5151

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/generator/GeneratorFunctionRootNode.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@
6363

6464
public class GeneratorFunctionRootNode extends PClosureFunctionRootNode {
6565
private final RootCallTarget callTarget;
66-
@CompilationFinal(dimensions = 1) private RootCallTarget[] callTargets;
66+
@CompilationFinal(dimensions = 1) private volatile RootCallTarget[] callTargets;
6767
private final FrameDescriptor frameDescriptor;
6868
private final GeneratorInfo generatorInfo;
6969
private final ExecutionCellSlots cellSlots;

0 commit comments

Comments
 (0)