Some changes about storing/initializing hash table values

- Remove HashTraits::Store() because it's always
    slot = std::forward(value).
  A type with complex storage method should use HashTranslator.

- Rename HashTranslator::Translate() to Store() to make its meaning
  clearer.

- Require the type to be trivially destructible when it uses
  DeletedValue() to avoid double destruction.

- Remove HashTableBucketInitializer::Initialize() because it's only
  called after AllocateTable() which has already initialized all
  entries.

Found one violation with the third change. It's not a regression
of recent HashTraits changes because assignment had been used in
ConstructDeletedValue before the changes. The violation doesn't seem
to have caused any problem probably because the internal sk_sp<> was
always nullptr in the hash keys.

Bug: 1374475
Change-Id: I4f3d0401bb3b3a4e1896c30d270204f08cb4c7d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4202669
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098651}

third_party/blink/renderer/core/dom/qualified_name.cc

@@ -59,9 +59,9 @@ struct QNameComponentsTranslator {
            data.components_.local_name_ == name->local_name_.Impl() &&
            data.components_.namespace_ == name->namespace_.Impl();
   }
-  static void Translate(QualifiedName::QualifiedNameImpl*& location,
-                        const QualifiedNameData& data,
-                        unsigned) {
+  static void Store(QualifiedName::QualifiedNameImpl*& location,
+                    const QualifiedNameData& data,
+                    unsigned) {
     const QualifiedNameComponents& components = data.components_;
     auto name = QualifiedName::QualifiedNameImpl::Create(
         components.prefix_, components.local_name_, components.namespace_,

third_party/blink/renderer/platform/graphics/skia/sk_image_info_hash.h

@@ -23,13 +23,13 @@ struct HashTraits<SkImageInfo> : GenericHashTraits<SkImageInfo> {
   }
 
   static const bool kEmptyValueIsZero = true;
-  static SkImageInfo EmptyValue() {
-    return SkImageInfo::Make(0, 0, kUnknown_SkColorType, kUnknown_SkAlphaType,
-                             nullptr);
+  static SkImageInfo EmptyValue() { return SkImageInfo::MakeUnknown(); }
+  static void ConstructDeletedValue(SkImageInfo& slot) {
+    new (NotNullTag::kNotNull, &slot)
+        SkImageInfo(SkImageInfo::MakeUnknown(-1, -1));
   }
-  static SkImageInfo DeletedValue() {
-    return SkImageInfo::Make(-1, -1, kUnknown_SkColorType, kUnknown_SkAlphaType,
-                             nullptr);
+  static bool IsDeletedValue(const SkImageInfo& value) {
+    return value == SkImageInfo::MakeUnknown(-1, -1);
   }
 };
 

third_party/blink/renderer/platform/heap/member.h

@@ -114,11 +114,6 @@ struct BaseMemberHashTraits : SimpleClassHashTraits<MemberType> {
 
   static PeekOutType Peek(const MemberType& value) { return value; }
 
-  template <typename U>
-  static void Store(const U& value, MemberType& storage) {
-    storage = value;
-  }
-
   static void ConstructDeletedValue(MemberType& slot) {
     slot = cppgc::kSentinelPointer;
   }

third_party/blink/renderer/platform/wtf/hash_map.h

@@ -324,9 +324,9 @@ struct HashMapTranslator {
     return KeyTraits::Equal(a, b);
   }
   template <typename T, typename U, typename V>
-  static void Translate(T& location, U&& key, V&& mapped) {
+  static void Store(T& location, U&& key, V&& mapped) {
     location.key = std::forward<U>(key);
-    ValueTraits::ValueTraits::Store(std::forward<V>(mapped), location.value);
+    location.value = std::forward<V>(mapped);
   }
 };
 
@@ -445,8 +445,7 @@ typename HashMap<T, U, V, W, X>::AddResult HashMap<T, U, V, W, X>::Set(
     //
     // It's safe to call std::forward again, because |mapped| isn't moved if
     // there's an existing entry.
-    MappedTraits::Store(std::forward<IncomingMappedType>(mapped),
-                        result.stored_value->value);
+    result.stored_value->value = std::forward<IncomingMappedType>(mapped);
   }
   return result;
 }

third_party/blink/renderer/platform/wtf/hash_set.h

@@ -119,7 +119,7 @@ class HashSet {
   // following function members:
   //   static unsigned GetHash(const T&);
   //   static bool Equal(const ValueType&, const T&);
-  //   static Translate(ValueType&, T&&, unsigned hash_code);
+  //   static Store(ValueType&, T&&, unsigned hash_code);
   template <typename HashTranslator, typename T>
   AddResult AddWithTranslator(T&&);
 
@@ -184,8 +184,8 @@ struct HashSetTranslatorAdapter {
     return Translator::Equal(a, b);
   }
   template <typename T, typename U, typename V>
-  static void Translate(T& location, U&& key, const V&, unsigned hash_code) {
-    Translator::Translate(location, std::forward<U>(key), hash_code);
+  static void Store(T& location, U&& key, const V&, unsigned hash_code) {
+    Translator::Store(location, std::forward<U>(key), hash_code);
   }
 };
 

third_party/blink/renderer/platform/wtf/hash_table.h

@@ -549,7 +549,7 @@ class IdentityHashTranslator {
     return KeyTraits::Equal(a, b);
   }
   template <typename T, typename U, typename V>
-  static void Translate(T& location, U&&, V&& value) {
+  static void Store(T& location, U&&, V&& value) {
     location = std::forward<V>(value);
   }
 };
@@ -710,12 +710,12 @@ class HashTable final
   // HashTranslator must have the following function members:
   //   static unsigned GetHash(const T&);
   //   static bool Equal(const ValueType&, const T&);
-  //   static void Translate(T& location, KeyType&&, ValueType&&);
+  //   static void Store(T& location, KeyType&&, ValueType&&);
   template <typename HashTranslator, typename T, typename Extra>
   AddResult insert(T&& key, Extra&&);
   // Similar to the above, but passes additional `unsigned hash_code`, which
   // is computed from `HashTranslator::GetHash(key)`, to HashTranslator method
-  //   static Translate(T&, KeyType&&, ValueType&&, unsigned hash_code);
+  //   static Store(T&, KeyType&&, ValueType&&, unsigned hash_code);
   // to avoid recomputation of the hash code when needed in the method.
   template <typename HashTranslator, typename T, typename Extra>
   AddResult InsertPassingHashCode(T&& key, Extra&&);
@@ -840,7 +840,6 @@ class HashTable final
   ValueType* Rehash(unsigned new_table_size, ValueType* entry);
   ValueType* Reinsert(ValueType&&);
 
-  static void InitializeBucket(ValueType& bucket);
   static void ReinitializeBucket(ValueType& bucket);
   static void DeleteBucket(ValueType& bucket) {
     bucket.~ValueType();
@@ -1133,14 +1132,12 @@ struct HashTableBucketInitializer {
   STATIC_ONLY(HashTableBucketInitializer);
   static_assert(!Traits::kEmptyValueIsZero);
 
-  static void Initialize(Value& bucket) {
+  static void Reinitialize(Value& bucket) {
     ConstructTraits<Value, Traits, Allocator>::ConstructAndNotifyElement(
         &bucket, Traits::EmptyValue());
     DCHECK(IsHashTraitsEmptyValue<Traits>(bucket));
   }
 
-  static void Reinitialize(Value& bucket) { Initialize(bucket); }
-
   template <typename HashTable>
   static Value* AllocateTable(unsigned size, size_t alloc_size) {
     Value* result =
@@ -1152,7 +1149,7 @@ struct HashTableBucketInitializer {
 
   static void InitializeTable(Value* table, unsigned size) {
     for (unsigned i = 0; i < size; i++) {
-      Initialize(table[i]);
+      Reinitialize(table[i]);
     }
   }
 };
@@ -1162,15 +1159,6 @@ struct HashTableBucketInitializer {
 template <typename Traits, typename Allocator, typename Value>
 struct HashTableBucketInitializer<Traits, Allocator, Value, true> {
   STATIC_ONLY(HashTableBucketInitializer);
-  static void Initialize(Value& bucket) {
-    // The memset to 0 looks like a slow operation but is optimized by the
-    // compilers.
-    //
-    // NOLINTNEXTLINE(bugprone-undefined-memory-manipulation)
-    memset(&bucket, 0, sizeof(bucket));
-    CheckEmptyValues(&bucket, 1);
-  }
-
   static void Reinitialize(Value& bucket) {
     // The memset to 0 looks like a slow operation but is optimized by the
     // compilers.
@@ -1207,17 +1195,6 @@ struct HashTableBucketInitializer<Traits, Allocator, Value, true> {
   }
 };
 
-template <typename Key,
-          typename Value,
-          typename Extractor,
-          typename Traits,
-          typename KeyTraits,
-          typename Allocator>
-inline void HashTable<Key, Value, Extractor, Traits, KeyTraits, Allocator>::
-    InitializeBucket(ValueType& bucket) {
-  HashTableBucketInitializer<Traits, Allocator, Value>::Initialize(bucket);
-}
-
 template <typename Key,
           typename Value,
           typename Extractor,
@@ -1303,8 +1280,8 @@ typename HashTable<Key, Value, Extractor, Traits, KeyTraits, Allocator>::
     --deleted_count_;
   }
 
-  HashTranslator::Translate(*entry, std::forward<T>(key),
-                            std::forward<Extra>(extra));
+  HashTranslator::Store(*entry, std::forward<T>(key),
+                        std::forward<Extra>(extra));
   DCHECK(!IsEmptyOrDeletedBucket(*entry));
   // Translate constructs an element so we need to notify using the trait. Avoid
   // doing that in the translator so that they can be easily customized.
@@ -1362,8 +1339,8 @@ typename HashTable<Key, Value, Extractor, Traits, KeyTraits, Allocator>::
     --deleted_count_;
   }
 
-  HashTranslator::Translate(*entry, std::forward<T>(key),
-                            std::forward<Extra>(extra), lookup_result.hash);
+  HashTranslator::Store(*entry, std::forward<T>(key),
+                        std::forward<Extra>(extra), lookup_result.hash);
   DCHECK(!IsEmptyOrDeletedBucket(*entry));
   // Translate constructs an element so we need to notify using the trait. Avoid
   // doing that in the translator so that they can be easily customized.
@@ -1659,7 +1636,8 @@ HashTable<Key, Value, Extractor, Traits, KeyTraits, Allocator>::ExpandBuffer(
       new_entry = &temporary_table[i];
     if (IsEmptyOrDeletedBucket(table_[i])) {
       DCHECK_NE(&table_[i], entry);
-      InitializeBucket(temporary_table[i]);
+      // All entries are initially empty. See AllocateTable().
+      DCHECK(IsEmptyBucket(temporary_table[i]));
     } else {
       Mover<ValueType, Allocator, Traits,
             Traits::template NeedsToForbidGCOnMove<>::value>::

third_party/blink/renderer/platform/wtf/hash_traits.h

@@ -82,6 +82,7 @@ struct GenericHashTraitsBase {
   using TraitType = T;
 
   // Type for functions that do not take ownership, such as contains.
+  // If overridden, the type must be assignable to T.
   using PeekInType = const T&;
 
   // Types for iterators.
@@ -90,11 +91,6 @@ struct GenericHashTraitsBase {
   using IteratorReferenceType = T&;
   using IteratorConstReferenceType = const T&;
 
-  template <typename IncomingValueType>
-  static void Store(IncomingValueType&& value, T& storage) {
-    storage = std::forward<IncomingValueType>(value);
-  }
-
   // Type for return value of functions that do not transfer ownership, such
   // as get.
   using PeekOutType = T;
@@ -156,10 +152,13 @@ struct GenericHashTraitsBase {
   // Checks if a given value is a deleted value. If this is defined, the hash
   // table will call this function (through IsHashTraitsDeletedValue()) to check
   // if a slot is deleted. Otherwise `v == DeletedValue()` will be used.
-  // This is for key typess only.
+  // This is for key types only.
   static bool IsDeletedValue(const T& v) = delete;
 
   // Constructs a deleted value in-place in the given memory space.
+  // When this is called, T's destructor on the slot has been called, so this
+  // function should not call destructor again (e.g. by assigning a value
+  // to `slot`), unless T is trivially destructible.
   // This must be defined if IsDeletedValue() is defined, and will be called
   // through ConstructHashTraitsDeletedValue(). Otherwise
   // `slot = DeletedValue()` will be used.
@@ -332,10 +331,6 @@ struct GenericHashTraits<scoped_refptr<P>>
   typedef scoped_refptr<P>& IteratorReferenceType;
   typedef const scoped_refptr<P>& IteratorConstReferenceType;
 
-  static void Store(scoped_refptr<P> value, scoped_refptr<P>& storage) {
-    storage = std::move(value);
-  }
-
   typedef P* PeekOutType;
   static PeekOutType Peek(const scoped_refptr<P>& value) { return value.get(); }
 
@@ -368,10 +363,6 @@ struct GenericHashTraits<std::unique_ptr<T>>
 
   using PeekInType = T*;
 
-  static void Store(std::unique_ptr<T>&& value, std::unique_ptr<T>& storage) {
-    storage = std::move(value);
-  }
-
   using PeekOutType = T*;
   static PeekOutType Peek(const std::unique_ptr<T>& value) {
     return value.get();
@@ -434,8 +425,7 @@ namespace internal {
 
 template <typename Traits, typename Enabled = void>
 struct HashTraitsEmptyValueChecker {
-  template <typename T>
-  static bool IsEmptyValue(const T& value) {
+  static bool IsEmptyValue(const typename Traits::TraitType& value) {
     return value == Traits::EmptyValue();
   }
 };
@@ -446,20 +436,18 @@ struct HashTraitsEmptyValueChecker<
         std::is_same_v<decltype(Traits::IsEmptyValue(
                            std::declval<typename Traits::TraitType>())),
                        bool>>> {
-  template <typename T>
-  static bool IsEmptyValue(const T& value) {
+  static bool IsEmptyValue(const typename Traits::TraitType& value) {
     return Traits::IsEmptyValue(value);
   }
 };
 
 template <typename Traits, typename Enabled = void>
 struct HashTraitsDeletedValueHelper {
-  template <typename T>
-  static bool IsDeletedValue(const T& value) {
+  static bool IsDeletedValue(const typename Traits::TraitType& value) {
     return value == Traits::DeletedValue();
   }
-  template <typename T>
-  static void ConstructDeletedValue(T& slot) {
+  static void ConstructDeletedValue(typename Traits::TraitType& slot) {
+    static_assert(std::is_trivially_destructible_v<typename Traits::TraitType>);
     slot = Traits::DeletedValue();
   }
 };
@@ -470,14 +458,12 @@ struct HashTraitsDeletedValueHelper<
         std::is_same_v<decltype(Traits::IsDeletedValue(
                            std::declval<typename Traits::TraitType>())),
                        bool>>> {
-  template <typename T>
-  static bool IsDeletedValue(const T& value) {
+  static bool IsDeletedValue(const typename Traits::TraitType& value) {
     return Traits::IsDeletedValue(value);
   }
   // Traits must also define ConstructDeletedValue() if it defines
   // IsDeletedValue().
-  template <typename T>
-  static void ConstructDeletedValue(T& slot) {
+  static void ConstructDeletedValue(typename Traits::TraitType& slot) {
     Traits::ConstructDeletedValue(slot);
   }
 };

third_party/blink/renderer/platform/wtf/text/atomic_string_table.cc

@@ -54,9 +54,9 @@ struct UCharBufferTranslator {
     return WTF::Equal(str, buf.characters(), buf.length());
   }
 
-  static void Translate(StringImpl*& location,
-                        const UCharBuffer& buf,
-                        unsigned hash) {
+  static void Store(StringImpl*& location,
+                    const UCharBuffer& buf,
+                    unsigned hash) {
     location = buf.CreateStringImpl().release();
     location->SetHash(hash);
     location->SetIsAtomic();
@@ -133,9 +133,9 @@ struct HashAndUTF8CharactersTranslator {
     return true;
   }
 
-  static void Translate(StringImpl*& location,
-                        const HashAndUTF8Characters& buffer,
-                        unsigned hash) {
+  static void Store(StringImpl*& location,
+                    const HashAndUTF8Characters& buffer,
+                    unsigned hash) {
     scoped_refptr<StringImpl> new_string;
     // If buffer contains only ASCII characters, the UTF-8 and UTF-16 lengths
     // are the same.
@@ -326,9 +326,9 @@ struct LCharBufferTranslator {
     return WTF::Equal(str, buf.characters(), buf.length());
   }
 
-  static void Translate(StringImpl*& location,
-                        const LCharBuffer& buf,
-                        unsigned hash) {
+  static void Store(StringImpl*& location,
+                    const LCharBuffer& buf,
+                    unsigned hash) {
     auto string = StringImpl::Create(buf.characters(), buf.length());
     location = string.release();
     location->SetHash(hash);