From 7c6cab995a1bf955992cde62adeb440a4e38e19e Mon Sep 17 00:00:00 2001 From: Teagan Strickland Date: Tue, 27 Aug 2019 10:00:18 +0000 Subject: [PATCH] [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 Reviewed-by: Martin Kustermann --- runtime/vm/hash_map.h | 23 +++++++++++ runtime/vm/hash_map_test.cc | 80 +++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/runtime/vm/hash_map.h b/runtime/vm/hash_map.h index 4b032f57874..04bfdd1b155 100644 --- a/runtime/vm/hash_map.h +++ b/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::Insert( } } +template +void BaseDirectChainedHashMap::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 bool BaseDirectChainedHashMap::Remove( typename KeyValueTrait::Key key) { diff --git a/runtime/vm/hash_map_test.cc b/runtime/vm/hash_map_test.cc index b2d8c00999d..a040869d646 100644 --- a/runtime/vm/hash_map_test.cc +++ b/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::Pair p1 = {str1, 1}; + CStringKeyValueTrait::Pair p2 = {str2, 2}; + CStringKeyValueTrait::Pair p3 = {str3, 3}; + + CStringMap 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