From 6bcb886bf83f4954d2076e03e1ed9d6b8b247565 Mon Sep 17 00:00:00 2001 From: Xianzhu Wang Date: Mon, 30 Jan 2023 17:08:24 +0000 Subject: [PATCH] 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 Reviewed-by: Kentaro Hara Cr-Commit-Position: refs/heads/main@{#1098651} --- .../blink/renderer/core/dom/qualified_name.cc | 6 +-- .../graphics/skia/sk_image_info_hash.h | 12 ++--- .../blink/renderer/platform/heap/member.h | 5 --- .../blink/renderer/platform/wtf/hash_map.h | 7 ++- .../blink/renderer/platform/wtf/hash_set.h | 6 +-- .../blink/renderer/platform/wtf/hash_table.h | 44 +++++-------------- .../blink/renderer/platform/wtf/hash_traits.h | 38 +++++----------- .../platform/wtf/text/atomic_string_table.cc | 18 ++++---- 8 files changed, 47 insertions(+), 89 deletions(-) diff --git a/third_party/blink/renderer/core/dom/qualified_name.cc b/third_party/blink/renderer/core/dom/qualified_name.cc index c00d80bf47c8bc..34ae1bb3225192 100644 --- a/third_party/blink/renderer/core/dom/qualified_name.cc +++ b/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_, diff --git a/third_party/blink/renderer/platform/graphics/skia/sk_image_info_hash.h b/third_party/blink/renderer/platform/graphics/skia/sk_image_info_hash.h index 232fb9b3a08300..4d02044d82e0df 100644 --- a/third_party/blink/renderer/platform/graphics/skia/sk_image_info_hash.h +++ b/third_party/blink/renderer/platform/graphics/skia/sk_image_info_hash.h @@ -23,13 +23,13 @@ struct HashTraits : GenericHashTraits { } 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); } }; diff --git a/third_party/blink/renderer/platform/heap/member.h b/third_party/blink/renderer/platform/heap/member.h index e2bda61c64601b..5a2d5f457d6eda 100644 --- a/third_party/blink/renderer/platform/heap/member.h +++ b/third_party/blink/renderer/platform/heap/member.h @@ -114,11 +114,6 @@ struct BaseMemberHashTraits : SimpleClassHashTraits { static PeekOutType Peek(const MemberType& value) { return value; } - template - static void Store(const U& value, MemberType& storage) { - storage = value; - } - static void ConstructDeletedValue(MemberType& slot) { slot = cppgc::kSentinelPointer; } diff --git a/third_party/blink/renderer/platform/wtf/hash_map.h b/third_party/blink/renderer/platform/wtf/hash_map.h index 80b136f8d192d5..d8d1e8475c8972 100644 --- a/third_party/blink/renderer/platform/wtf/hash_map.h +++ b/third_party/blink/renderer/platform/wtf/hash_map.h @@ -324,9 +324,9 @@ struct HashMapTranslator { return KeyTraits::Equal(a, b); } template - static void Translate(T& location, U&& key, V&& mapped) { + static void Store(T& location, U&& key, V&& mapped) { location.key = std::forward(key); - ValueTraits::ValueTraits::Store(std::forward(mapped), location.value); + location.value = std::forward(mapped); } }; @@ -445,8 +445,7 @@ typename HashMap::AddResult HashMap::Set( // // It's safe to call std::forward again, because |mapped| isn't moved if // there's an existing entry. - MappedTraits::Store(std::forward(mapped), - result.stored_value->value); + result.stored_value->value = std::forward(mapped); } return result; } diff --git a/third_party/blink/renderer/platform/wtf/hash_set.h b/third_party/blink/renderer/platform/wtf/hash_set.h index fb91f92e4b655f..5923d347ecc15e 100644 --- a/third_party/blink/renderer/platform/wtf/hash_set.h +++ b/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 AddResult AddWithTranslator(T&&); @@ -184,8 +184,8 @@ struct HashSetTranslatorAdapter { return Translator::Equal(a, b); } template - static void Translate(T& location, U&& key, const V&, unsigned hash_code) { - Translator::Translate(location, std::forward(key), hash_code); + static void Store(T& location, U&& key, const V&, unsigned hash_code) { + Translator::Store(location, std::forward(key), hash_code); } }; diff --git a/third_party/blink/renderer/platform/wtf/hash_table.h b/third_party/blink/renderer/platform/wtf/hash_table.h index b21447753438d3..e11a262fd3b0d7 100644 --- a/third_party/blink/renderer/platform/wtf/hash_table.h +++ b/third_party/blink/renderer/platform/wtf/hash_table.h @@ -549,7 +549,7 @@ class IdentityHashTranslator { return KeyTraits::Equal(a, b); } template - static void Translate(T& location, U&&, V&& value) { + static void Store(T& location, U&&, V&& value) { location = std::forward(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 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 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::ConstructAndNotifyElement( &bucket, Traits::EmptyValue()); DCHECK(IsHashTraitsEmptyValue(bucket)); } - static void Reinitialize(Value& bucket) { Initialize(bucket); } - template 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 struct HashTableBucketInitializer { 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 { } }; -template -inline void HashTable:: - InitializeBucket(ValueType& bucket) { - HashTableBucketInitializer::Initialize(bucket); -} - template :: --deleted_count_; } - HashTranslator::Translate(*entry, std::forward(key), - std::forward(extra)); + HashTranslator::Store(*entry, std::forward(key), + std::forward(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:: --deleted_count_; } - HashTranslator::Translate(*entry, std::forward(key), - std::forward(extra), lookup_result.hash); + HashTranslator::Store(*entry, std::forward(key), + std::forward(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::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::value>:: diff --git a/third_party/blink/renderer/platform/wtf/hash_traits.h b/third_party/blink/renderer/platform/wtf/hash_traits.h index cdc1242ddb824d..a3a59f228e7d45 100644 --- a/third_party/blink/renderer/platform/wtf/hash_traits.h +++ b/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 - static void Store(IncomingValueType&& value, T& storage) { - storage = std::forward(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> typedef scoped_refptr

& IteratorReferenceType; typedef const scoped_refptr

& IteratorConstReferenceType; - static void Store(scoped_refptr

value, scoped_refptr

& storage) { - storage = std::move(value); - } - typedef P* PeekOutType; static PeekOutType Peek(const scoped_refptr

& value) { return value.get(); } @@ -368,10 +363,6 @@ struct GenericHashTraits> using PeekInType = T*; - static void Store(std::unique_ptr&& value, std::unique_ptr& storage) { - storage = std::move(value); - } - using PeekOutType = T*; static PeekOutType Peek(const std::unique_ptr& value) { return value.get(); @@ -434,8 +425,7 @@ namespace internal { template struct HashTraitsEmptyValueChecker { - template - 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())), bool>>> { - template - static bool IsEmptyValue(const T& value) { + static bool IsEmptyValue(const typename Traits::TraitType& value) { return Traits::IsEmptyValue(value); } }; template struct HashTraitsDeletedValueHelper { - template - static bool IsDeletedValue(const T& value) { + static bool IsDeletedValue(const typename Traits::TraitType& value) { return value == Traits::DeletedValue(); } - template - static void ConstructDeletedValue(T& slot) { + static void ConstructDeletedValue(typename Traits::TraitType& slot) { + static_assert(std::is_trivially_destructible_v); slot = Traits::DeletedValue(); } }; @@ -470,14 +458,12 @@ struct HashTraitsDeletedValueHelper< std::is_same_v())), bool>>> { - template - 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 - static void ConstructDeletedValue(T& slot) { + static void ConstructDeletedValue(typename Traits::TraitType& slot) { Traits::ConstructDeletedValue(slot); } }; diff --git a/third_party/blink/renderer/platform/wtf/text/atomic_string_table.cc b/third_party/blink/renderer/platform/wtf/text/atomic_string_table.cc index 5b1a5dd8403261..bd37697d55eefc 100644 --- a/third_party/blink/renderer/platform/wtf/text/atomic_string_table.cc +++ b/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 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);