ObjectHashMap: move remove into a node

Author: steve-s

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

@@ -93,10 +93,6 @@ public boolean execute(Frame frame, Object a, Object b) {
         }
     }
 
-    private static final ObjectHashMap.PutProfiles PUT_PROFILES = new PutProfiles(false, new EqNodeStub());
-    private static final ObjectHashMap.GetProfiles GET_PROFILES = new GetProfiles(false, new EqNodeStub());
-    private static final ObjectHashMap.RemoveProfiles RM_PROFILES = new RemoveProfiles(false, new EqNodeStub());
-
     @Test
     public void testCollisionsByPuttingManyKeysWithSameHash() {
         ObjectHashMap map = new ObjectHashMap();
@@ -111,7 +107,7 @@ public void testCollisionsByPuttingManyKeysWithSameHash() {
         for (int i = 0; i < 55; i++) {
             DictKey key = expected.keySet().stream().skip(22).findFirst().get();
             expected.remove(key);
-            map.remove(null, key, 42, RM_PROFILES);
+            remove(map, key, 42);
             assertEqual(i, expected, map);
         }
         for (int i = 0; i < 10; i++) {
@@ -134,7 +130,7 @@ public void testCollisionsByPuttingAndRemovingTheSameKey() {
             expected.put(key, value);
             assertEqual(i, expected, map);
 
-            map.remove(null, key, 42, RM_PROFILES);
+            remove(map, key, 42);
             expected.remove(key);
             assertEqual(i, expected, map);
         }
@@ -153,7 +149,7 @@ public void testCollisionsByPuttingAndRemovingTheSameKeys() {
             assertEqual(i, expected, map);
 
             final DictKey toRemove = keys[(i + 1) % keys.length];
-            map.remove(null, toRemove, toRemove.hash, RM_PROFILES);
+            remove(map, toRemove, toRemove.hash);
             expected.remove(toRemove);
             assertEqual(i, expected, map);
         }
@@ -215,14 +211,14 @@ private static void removeAll(ObjectHashMap map) {
             keys.add((Long) key);
         }
         for (Long key : keys) {
-            map.remove(null, key, PyObjectHashNode.hash(key), RM_PROFILES);
+            remove(map, key, PyObjectHashNode.hash(key));
             assertNull(get(map, key, PyObjectHashNode.hash(key)));
         }
     }
 
     private static void removeAll(ObjectHashMap map, LinkedHashMap<Long, Object> expected) {
         for (Long key : expected.keySet().toArray(new Long[0])) {
-            map.remove(null, key, PyObjectHashNode.hash(key), RM_PROFILES);
+            remove(map, key, PyObjectHashNode.hash(key));
             expected.remove(key);
             assertEqual(Long.toString(key), expected, map);
         }
@@ -232,7 +228,7 @@ private static void removeValues(ObjectHashMap map, LinkedHashMap<Long, Object>
         for (int i = 0; i < count; i++) {
             int index = rand.nextInt(expected.size() - 1);
             long key = expected.keySet().stream().skip(index).findFirst().get();
-            map.remove(null, key, PyObjectHashNode.hash(key), RM_PROFILES);
+            remove(map, key, PyObjectHashNode.hash(key));
             expected.remove(key);
             assertEqual(i, expected, map);
         }
@@ -327,6 +323,13 @@ private static Object get(ObjectHashMap map, Object key, long hash) {
                 new EqNodeStub());
     }
 
+    private static void remove(ObjectHashMap map, Object key, long hash) {
+        ObjectHashMap.RemoveNode.doRemove(null, map, key, hash,//
+                ConditionProfile.getUncached(), ConditionProfile.getUncached(), ConditionProfile.getUncached(),//
+                ConditionProfile.getUncached(), BranchProfile.getUncached(), ConditionProfile.getUncached(),//
+                new EqNodeStub());
+    }
+
     private static void put(ObjectHashMap map, Object key, long hash, Object value) {
         ObjectHashMap.PutNode.doPut(null, map, key, hash, value,//
                 ConditionProfile.getUncached(), ConditionProfile.getUncached(), ConditionProfile.getUncached(),//

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

@@ -300,11 +300,11 @@ static HashingStorage generic(EconomicMapStorage self, HashingStorage other,
 
     @ExportMessage
     HashingStorage delItemWithState(Object key, ThreadState state,
-                    @Cached ObjectHashMap.RemoveProfiles profiles,
+                    @Cached ObjectHashMap.RemoveNode removeNode,
                     @Shared("hashNode") @Cached PyObjectHashNode hashNode,
                     @Shared("gotState") @Cached ConditionProfile gotState) {
         VirtualFrame frame = gotState.profile(state == null) ? null : PArguments.frameForCall(state);
-        map.remove(frame, key, hashNode.execute(frame, key), profiles);
+        removeNode.remove(state, map, key, hashNode.execute(frame, key));
         return this;
     }
 

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

@@ -702,63 +702,78 @@ private boolean needsCompaction() {
         return dummyCnt > quarterOfUsable;
     }
 
-    public void remove(VirtualFrame frame, Object key, long keyHash, RemoveProfiles profiles) {
-        assert checkInternalState();
-        if (CompilerDirectives.injectBranchProbability(SLOWPATH_PROBABILITY, needsCompaction())) {
-            profiles.compactProfile.enter();
-            compact();
-        }
-        // Note: CPython is not shrinking the capacity of the hash table on delete, we do the same
-        int compactIndex = getIndex(keyHash);
-        int index = indices[compactIndex];
-        if (profiles.foundNullKey.profile(index == EMPTY_INDEX)) {
-            return; // not found
+
+    @GenerateUncached
+    public abstract static class RemoveNode extends Node {
+        public final void remove(ThreadState state, ObjectHashMap map, DictKey key) {
+            execute(state, map, key.getValue(), key.getPythonHash());
         }
 
-        int unwrappedIndex = unwrapIndex(index);
-        if (profiles.foundEqKey.profile(index != DUMMY_INDEX && keysEqual(frame, unwrappedIndex, key, keyHash, profiles))) {
-            indices[compactIndex] = DUMMY_INDEX;
-            setValue(unwrappedIndex, null);
-            setKey(unwrappedIndex, null);
-            size--;
-            return;
+        public final void remove(ThreadState state, ObjectHashMap map, Object key, long keyHash) {
+            execute(state, map, key, keyHash);
         }
 
-        // collision: intentionally counted loop
-        long perturb = keyHash;
-        int searchLimit = getBucketsCount() + PERTURB_SHIFTS_COUT;
-        int i = 0;
-        try {
-            for (; CompilerDirectives.injectBranchProbability(0, i < searchLimit); i++) {
-                perturb >>>= PERTURB_SHIFT;
-                compactIndex = nextIndex(compactIndex, perturb);
-                index = indices[compactIndex];
-                if (profiles.collisionFoundNoValue.profile(index == EMPTY_INDEX)) {
-                    return; // not found
-                }
-                unwrappedIndex = unwrapIndex(index);
-                if (profiles.collisionFoundEqKey.profile(index != DUMMY_INDEX && keysEqual(frame, unwrappedIndex, key, keyHash, profiles))) {
-                    indices[compactIndex] = DUMMY_INDEX;
-                    setValue(unwrappedIndex, null);
-                    setKey(unwrappedIndex, null);
-                    size--;
-                    return;
-                }
+        abstract void execute(ThreadState state, ObjectHashMap map, Object key, long keyHash);
+
+        // "public" for testing...
+        @Specialization
+        public static void doRemove(ThreadState state, ObjectHashMap map, Object key, long keyHash,
+                                   @Cached("createCountingProfile()") ConditionProfile foundNullKey,
+                                   @Cached("createCountingProfile()") ConditionProfile foundEqKey,
+                                   @Cached("createCountingProfile()") ConditionProfile collisionFoundNoValue,
+                                   @Cached("createCountingProfile()") ConditionProfile collisionFoundEqKey,
+                                   @Cached BranchProfile compactProfile,
+                                   @Cached ConditionProfile hasState,
+                                   @Cached PyObjectRichCompareBool.EqNode eqNode) {
+            assert map.checkInternalState();
+            if (CompilerDirectives.injectBranchProbability(SLOWPATH_PROBABILITY, map.needsCompaction())) {
+                compactProfile.enter();
+                map.compact();
+            }
+            // Note: CPython is not shrinking the capacity of the hash table on delete, we do the same
+            int compactIndex = map.getIndex(keyHash);
+            int index = map.indices[compactIndex];
+            if (foundNullKey.profile(index == EMPTY_INDEX)) {
+                return; // not found
             }
-        } finally {
-            LoopNode.reportLoopCount(profiles, i);
-        }
-        // all values are dummies? Not possible, since we should have compacted the
-        // hashes/keysAndValues arrays at the top
-        throw CompilerDirectives.shouldNotReachHere();
-    }
 
-    private boolean keysEqual(Frame frame, int index, Object key, long keyHash, GetProfiles profiles) {
-        return hashes[index] == keyHash && profiles.eqNode.execute(frame, getKey(index), key);
-    }
+            int unwrappedIndex = unwrapIndex(index);
+            if (foundEqKey.profile(index != DUMMY_INDEX && map.keysEqual(state, unwrappedIndex, key, keyHash, eqNode, hasState))) {
+                map.indices[compactIndex] = DUMMY_INDEX;
+                map.setValue(unwrappedIndex, null);
+                map.setKey(unwrappedIndex, null);
+                map.size--;
+                return;
+            }
 
-    private boolean keysEqual(Frame frame, int index, Object key, long keyHash, PyObjectRichCompareBool.EqNode eqNode) {
-        return hashes[index] == keyHash && eqNode.execute(frame, getKey(index), key);
+            // collision: intentionally counted loop
+            long perturb = keyHash;
+            int searchLimit = map.getBucketsCount() + PERTURB_SHIFTS_COUT;
+            int i = 0;
+            try {
+                for (; CompilerDirectives.injectBranchProbability(0, i < searchLimit); i++) {
+                    perturb >>>= PERTURB_SHIFT;
+                    compactIndex = map.nextIndex(compactIndex, perturb);
+                    index = map.indices[compactIndex];
+                    if (collisionFoundNoValue.profile(index == EMPTY_INDEX)) {
+                        return; // not found
+                    }
+                    unwrappedIndex = unwrapIndex(index);
+                    if (collisionFoundEqKey.profile(index != DUMMY_INDEX && map.keysEqual(state, unwrappedIndex, key, keyHash, eqNode, hasState))) {
+                        map.indices[compactIndex] = DUMMY_INDEX;
+                        map.setValue(unwrappedIndex, null);
+                        map.setKey(unwrappedIndex, null);
+                        map.size--;
+                        return;
+                    }
+                }
+            } finally {
+                LoopNode.reportLoopCount(eqNode, i);
+            }
+            // all values are dummies? Not possible, since we should have compacted the
+            // hashes/keysAndValues arrays at the top
+            throw CompilerDirectives.shouldNotReachHere();
+        }
     }
 
     private boolean keysEqual(ThreadState state, int index, Object key, long keyHash, PyObjectRichCompareBool.EqNode eqNode, ConditionProfile hasState) {