From 94e97e528dba8ba86ef70797d5ef83b9fbf80be0 Mon Sep 17 00:00:00 2001 From: Nikolai Tillmann Date: Wed, 22 Nov 2023 14:10:48 -0800 Subject: [PATCH] harmonize insert/emplace on InsertOnlyConcurrent* collections Summary: Besides helping with the next diff, this removes the need for look-ups immediately after inserting an element. This is a behavior-preserving change. Reviewed By: agampe Differential Revision: D51519942 fbshipit-source-id: 08e64180aa037d54f3f1bcc40bd98f65cfff72ec --- libredex/ConcurrentContainers.h | 89 +++++++++++++++++++++----------- libredex/KeepReason.cpp | 12 ++--- libredex/MethodOverrideGraph.cpp | 31 +++++------ libredex/Purity.cpp | 3 +- 4 files changed, 82 insertions(+), 53 deletions(-) diff --git a/libredex/ConcurrentContainers.h b/libredex/ConcurrentContainers.h index 1fa08d8d4e3..b9701cb24d6 100644 --- a/libredex/ConcurrentContainers.h +++ b/libredex/ConcurrentContainers.h @@ -1562,24 +1562,45 @@ class InsertOnlyConcurrentMap final const Value& at_unsafe(const Key& key) const { return at(key); } /* - * The Boolean return value denotes whether the insertion took place. - * This operation is always thread-safe. - * - * Note that while the STL containers' insert() methods return both an - * iterator and a boolean success value, we only return the boolean value - * here as any operations on a returned iterator are not guaranteed to be - * thread-safe. + * Returns a pair consisting of a pointer on the inserted element (or the + * element that prevented the insertion) and a boolean denoting whether the + * insertion took place. This operation is always thread-safe. */ - bool insert(const KeyValuePair& entry) { + std::pair insert(const KeyValuePair& entry) { size_t slot = Hash()(entry.first) % n_slots; auto& map = this->get_container(slot); - return map.try_insert(entry).success; + auto insertion_result = map.try_insert(entry); + return std::make_pair(&insertion_result.stored_value_ptr->second, + insertion_result.success); } - bool insert(KeyValuePair&& entry) { + /* + * Returns a pair consisting of a pointer on the inserted element (or the + * element that prevented the insertion) and a boolean denoting whether the + * insertion took place. This operation is always thread-safe. + */ + std::pair insert(KeyValuePair&& entry) { size_t slot = Hash()(entry.first) % n_slots; auto& map = this->get_container(slot); - return map.try_insert(std::forward(entry)).success; + auto insertion_result = map.try_insert(std::move(entry)); + return std::make_pair(&insertion_result.stored_value_ptr->second, + insertion_result.success); + } + + std::pair insert_unsafe(const KeyValuePair& entry) { + size_t slot = Hash()(entry.first) % n_slots; + auto& map = this->get_container(slot); + auto insertion_result = map.try_insert(entry); + return std::make_pair(&insertion_result.stored_value_ptr->second, + insertion_result.success); + } + + std::pair insert_unsafe(KeyValuePair entry) { + size_t slot = Hash()(entry.first) % n_slots; + auto& map = this->get_container(slot); + auto insertion_result = map.try_insert(std::move(entry)); + return std::make_pair(&insertion_result.stored_value_ptr->second, + insertion_result.success); } /* @@ -1601,28 +1622,12 @@ class InsertOnlyConcurrentMap final } } - void insert_or_assign_unsafe(const KeyValuePair& entry) { - size_t slot = Hash()(entry.first) % n_slots; - auto& map = this->get_container(slot); - auto insertion_result = map.try_emplace(entry); - if (insertion_result.success) { - return; - } - auto* constructed_value = insertion_result.incidentally_constructed_value(); - if (constructed_value) { - insertion_result.stored_value_ptr->second = - std::move(constructed_value->second); - } else { - insertion_result.stored_value_ptr->second = entry.second; - } - } - - void insert_or_assign_unsafe(KeyValuePair&& entry) { + std::pair insert_or_assign_unsafe(KeyValuePair&& entry) { size_t slot = Hash()(entry.first) % n_slots; auto& map = this->get_container(slot); auto insertion_result = map.try_emplace(std::forward(entry)); if (insertion_result.success) { - return; + return std::make_pair(&insertion_result.stored_value_ptr->second, true); } auto* constructed_value = insertion_result.incidentally_constructed_value(); if (constructed_value) { @@ -1632,17 +1637,20 @@ class InsertOnlyConcurrentMap final insertion_result.stored_value_ptr->second = std::forward(entry.second); } + return std::make_pair(&insertion_result.stored_value_ptr->second, false); } /* * This operation is always thread-safe. */ template - bool emplace(Args&&... args) { + std::pair emplace(Args&&... args) { KeyValuePair entry(std::forward(args)...); size_t slot = Hash()(entry.first) % n_slots; auto& map = this->get_container(slot); - return map.try_insert(std::move(entry)).success; + auto insertion_result = map.try_insert(std::move(entry)); + return std::make_pair(&insertion_result.stored_value_ptr->second, + insertion_result.success); } template @@ -2080,6 +2088,11 @@ class InsertOnlyConcurrentSet final return {insertion_result.stored_value_ptr, insertion_result.success}; } + /* + * Returns a pair consisting of a pointer on the inserted element (or the + * element that prevented the insertion) and a boolean denoting whether the + * insertion took place. This operation is always thread-safe. + */ std::pair insert(Key&& key) { size_t slot = Hash()(key) % n_slots; auto& set = this->get_container(slot); @@ -2087,6 +2100,20 @@ class InsertOnlyConcurrentSet final return {insertion_result.stored_value_ptr, insertion_result.success}; } + std::pair insert_unsafe(const Key& key) { + size_t slot = Hash()(key) % n_slots; + auto& set = this->get_container(slot); + auto insertion_result = set.try_insert(key); + return {insertion_result.stored_value_ptr, insertion_result.success}; + } + + std::pair insert_unsafe(Key&& key) { + size_t slot = Hash()(key) % n_slots; + auto& set = this->get_container(slot); + auto insertion_result = set.try_insert(std::forward(key)); + return {insertion_result.stored_value_ptr, insertion_result.success}; + } + /* * Return a pointer on the element, or `nullptr` if the element is not in the * set. This operation is always thread-safe. diff --git a/libredex/KeepReason.cpp b/libredex/KeepReason.cpp index c73db8b2c36..b331c6b2dde 100644 --- a/libredex/KeepReason.cpp +++ b/libredex/KeepReason.cpp @@ -53,8 +53,7 @@ namespace { // Lint will complain about this, but it is better than having to // forward-declare all of concurrent containers. -std::unique_ptr> s_keep_reasons{nullptr}; @@ -66,17 +65,18 @@ bool Reason::s_record_keep_reasons = false; void Reason::set_record_keep_reasons(bool v) { s_record_keep_reasons = v; if (v && s_keep_reasons == nullptr) { - s_keep_reasons = std::make_unique>(); } } Reason* Reason::try_insert(std::unique_ptr to_insert) { - if (s_keep_reasons->emplace(to_insert.get(), to_insert.get())) { + auto [reason_ptr, emplaced] = s_keep_reasons->insert(to_insert.get()); + if (emplaced) { return to_insert.release(); } - return s_keep_reasons->at(to_insert.get()); + return const_cast(*reason_ptr); } void Reason::release_keep_reasons() { s_keep_reasons.reset(); } diff --git a/libredex/MethodOverrideGraph.cpp b/libredex/MethodOverrideGraph.cpp index 65b52f9a5eb..b976555739e 100644 --- a/libredex/MethodOverrideGraph.cpp +++ b/libredex/MethodOverrideGraph.cpp @@ -96,7 +96,7 @@ class GraphBuilder { } private: - ClassSignatureMap analyze_non_interface(const DexClass* cls) { + const ClassSignatureMap& analyze_non_interface(const DexClass* cls) { always_assert(!is_interface(cls)); auto* res = m_class_signature_maps.get(cls); if (res) { @@ -155,7 +155,9 @@ class GraphBuilder { &class_signatures.unimplemented); } - if (m_class_signature_maps.emplace(cls, class_signatures)) { + auto [map_ptr, emplaced] = + m_class_signature_maps.emplace(cls, class_signatures); + if (emplaced) { // Mark all overriding methods as reachable via their parent method ref. for (auto* method : cls->get_vmethods()) { const auto& overridden_set = @@ -181,12 +183,12 @@ class GraphBuilder { } } } - return class_signatures; } - return m_class_signature_maps.at(cls); + + return *map_ptr; } - SignatureMap analyze_interface(const DexClass* cls) { + const SignatureMap& analyze_interface(const DexClass* cls) { always_assert(is_interface(cls)); auto* res = m_interface_signature_maps.get(cls); if (res) { @@ -199,7 +201,9 @@ class GraphBuilder { update_signature_map(method, MethodSet{method}, &interface_signatures); } - if (m_interface_signature_maps.emplace(cls, interface_signatures)) { + auto [map_ptr, emplaced] = + m_interface_signature_maps.emplace(cls, interface_signatures); + if (emplaced) { for (auto* method : cls->get_vmethods()) { const auto& overridden_set = inherited_interface_signatures.at(method->get_name()) @@ -226,13 +230,12 @@ class GraphBuilder { method, /* overriding_is_interface */ true); } } - - return interface_signatures; } - return m_interface_signature_maps.at(cls); + + return *map_ptr; } - SignatureMap unify_super_interface_signatures(const DexClass* cls) { + const SignatureMap& unify_super_interface_signatures(const DexClass* cls) { auto* type_list = cls->get_interfaces(); auto* res = m_unified_interfaces_signature_maps.get(type_list); if (res) { @@ -248,11 +251,9 @@ class GraphBuilder { } } - if (m_unified_interfaces_signature_maps.emplace( - type_list, super_interface_signatures)) { - return super_interface_signatures; - } - return m_unified_interfaces_signature_maps.at(type_list); + auto [map_ptr, _] = m_unified_interfaces_signature_maps.emplace( + type_list, super_interface_signatures); + return *map_ptr; } std::unique_ptr m_graph; diff --git a/libredex/Purity.cpp b/libredex/Purity.cpp index b00730b4472..c740648eb95 100644 --- a/libredex/Purity.cpp +++ b/libredex/Purity.cpp @@ -495,7 +495,8 @@ get_wto_successors( } } } - auto emplaced = concurrent_cache->emplace(m, std::move(successors)); + auto [_, emplaced] = + concurrent_cache->emplace(m, std::move(successors)); always_assert(emplaced); }, wto_nodes);