[vm] Adds Update method to hash maps.

The Insert method runs in constant time, but inserting multiple
pairs with the same key results in all pairs being added to the
map. Remove only removes the most recent pair with a key equal
to its argument, so this means calling Remove after an Insert
does not guarantee that Lookup for a key will fail.

The new Update method, when used instead of Insert, guarantees
that the map contains at most one pair for a set of equal keys.
This means that Lookup is guaranteed to fail after a Remove
call, but the Update method requires walking the existing
chain of entries for the key's hash value.

(In practice, our code tends to call Lookup before Insert to
avoid adding pairs with equal keys more than once, but this
practice does mean that updating the value returned by Lookup
cannot be done without first removing the old entry. This new
method simplifies that workflow if used consistently.)

Change-Id: I2944472a7fc74c1de41cb25eb9469a7c7e491ceb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/114069
Commit-Queue: Teagan Strickland <sstrickl@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>

runtime/vm/hash_map.h

@@ -39,6 +39,15 @@ class BaseDirectChainedHashMap : public B {
   void Insert(typename KeyValueTrait::Pair kv);
   bool Remove(typename KeyValueTrait::Key key);
 
+  // If a pair already exists in the map with an equal key, replace that pair
+  // with this one. Otherwise, insert the pair as a new entry.
+  //
+  // Note: Insert operates in constant time, while Update must walk the chained
+  // entries for a given hash value, checking keys for equality. However, if
+  // multiple value updates are needed for the same key, only using Update
+  // guarantees constant space usage whereas Insert does not.
+  void Update(typename KeyValueTrait::Pair kv);
+
   typename KeyValueTrait::Value LookupValue(
       typename KeyValueTrait::Key key) const;
 
@@ -314,6 +323,20 @@ void BaseDirectChainedHashMap<KeyValueTrait, B, Allocator>::Insert(
   }
 }
 
+template <typename KeyValueTrait, typename B, typename Allocator>
+void BaseDirectChainedHashMap<KeyValueTrait, B, Allocator>::Update(
+    typename KeyValueTrait::Pair kv) {
+  const typename KeyValueTrait::Value kNoValue =
+      KeyValueTrait::ValueOf(typename KeyValueTrait::Pair());
+
+  ASSERT(KeyValueTrait::ValueOf(kv) != kNoValue);
+  if (auto const old_kv = Lookup(KeyValueTrait::KeyOf(kv))) {
+    *old_kv = kv;
+  } else {
+    Insert(kv);
+  }
+}
+
 template <typename KeyValueTrait, typename B, typename Allocator>
 bool BaseDirectChainedHashMap<KeyValueTrait, B, Allocator>::Remove(
     typename KeyValueTrait::Key key) {

runtime/vm/hash_map_test.cc

@@ -220,4 +220,84 @@ TEST_CASE(CStringMap) {
   free(str1);
 }
 
+TEST_CASE(CStringMapUpdate) {
+  const char* const kConst1 = "test";
+  const char* const kConst2 = "test 2";
+
+  char* str1 = OS::SCreate(nullptr, "%s", kConst1);
+  char* str2 = OS::SCreate(nullptr, "%s", kConst2);
+  char* str3 = OS::SCreate(nullptr, "%s", kConst1);
+  char* str4 = OS::SCreate(nullptr, "%s", kConst1);  // Only used for lookup.
+
+  // Make sure these strings are pointer-distinct, but C-string-equal.
+  EXPECT_NE(str1, str3);
+  EXPECT_NE(str1, str4);
+  EXPECT_NE(str3, str4);
+  EXPECT_STREQ(str1, str3);
+  EXPECT_STREQ(str1, str4);
+
+  CStringKeyValueTrait<intptr_t>::Pair p1 = {str1, 1};
+  CStringKeyValueTrait<intptr_t>::Pair p2 = {str2, 2};
+  CStringKeyValueTrait<intptr_t>::Pair p3 = {str3, 3};
+
+  CStringMap<intptr_t> map;
+  EXPECT(map.IsEmpty());
+
+  map.Update(p1);
+  EXPECT_NOTNULL(map.Lookup(str1));
+  EXPECT_EQ(p1.value, map.LookupValue(str1));
+  EXPECT_NULLPTR(map.Lookup(str2));
+  EXPECT_NOTNULL(map.Lookup(str3));
+  EXPECT_EQ(p1.value, map.LookupValue(str3));
+  EXPECT_NOTNULL(map.Lookup(str4));
+  EXPECT_EQ(p1.value, map.LookupValue(str4));
+
+  map.Update(p2);
+  EXPECT_NOTNULL(map.Lookup(str1));
+  EXPECT_EQ(p1.value, map.LookupValue(str1));
+  EXPECT_NOTNULL(map.Lookup(str2));
+  EXPECT_EQ(p2.value, map.LookupValue(str2));
+  EXPECT_NOTNULL(map.Lookup(str3));
+  EXPECT_EQ(p1.value, map.LookupValue(str3));
+  EXPECT_NOTNULL(map.Lookup(str4));
+  EXPECT_EQ(p1.value, map.LookupValue(str4));
+
+  // Check Lookup after Update.
+  map.Update(p3);
+  EXPECT_NOTNULL(map.Lookup(str1));
+  EXPECT_EQ(p3.value, map.LookupValue(str1));
+  EXPECT_NOTNULL(map.Lookup(str2));
+  EXPECT_EQ(p2.value, map.LookupValue(str2));
+  EXPECT_NOTNULL(map.Lookup(str3));
+  EXPECT_EQ(p3.value, map.LookupValue(str3));
+  EXPECT_NOTNULL(map.Lookup(str4));
+  EXPECT_EQ(p3.value, map.LookupValue(str4));
+
+  // Check that single Remove after only Updates ensures Lookup fails after.
+  EXPECT(map.Remove(str3));
+  EXPECT_NULLPTR(map.Lookup(str1));
+  EXPECT_NOTNULL(map.Lookup(str2));
+  EXPECT_EQ(p2.value, map.LookupValue(str2));
+  EXPECT_NULLPTR(map.Lookup(str3));
+  EXPECT_NULLPTR(map.Lookup(str4));
+
+  EXPECT(!map.Remove(str3));
+  EXPECT(map.Remove(str2));
+  EXPECT(map.IsEmpty());
+
+  // Quick double-check that these weren't side-effected by the implementation
+  // of hash maps (p1 especially).
+  EXPECT_EQ(str1, p1.key);
+  EXPECT_EQ(1, p1.value);
+  EXPECT_EQ(str2, p2.key);
+  EXPECT_EQ(2, p2.value);
+  EXPECT_EQ(str3, p3.key);
+  EXPECT_EQ(3, p3.value);
+
+  free(str4);
+  free(str3);
+  free(str2);
+  free(str1);
+}
+
 }  // namespace dart