From 14c6648c71b4b8a12ac0905ff23fcd4de7f0556f Mon Sep 17 00:00:00 2001 From: Oliver Reiche Date: Mon, 13 Jun 2022 13:30:13 +0200 Subject: multithreading: Add AtomicValue to atomically set/get value ... and use it to replace the commonly used pattern in Expression, LinkedMap, and GitTreeEntry. Furthermore, remove assignment operators for Expression and LinkedMap as those are considered to be used in an immutable manner anyway. --- src/buildtool/build_engine/expression/TARGETS | 4 +- .../build_engine/expression/expression.cpp | 11 +---- .../build_engine/expression/expression.hpp | 25 +++------- .../build_engine/expression/linked_map.hpp | 49 ++++---------------- src/buildtool/file_system/TARGETS | 2 +- src/buildtool/file_system/git_tree.cpp | 26 +++-------- src/buildtool/file_system/git_tree.hpp | 5 +- src/buildtool/multithreading/TARGETS | 7 +++ src/buildtool/multithreading/atomic_value.hpp | 54 ++++++++++++++++++++++ 9 files changed, 88 insertions(+), 95 deletions(-) create mode 100644 src/buildtool/multithreading/atomic_value.hpp (limited to 'src') diff --git a/src/buildtool/build_engine/expression/TARGETS b/src/buildtool/build_engine/expression/TARGETS index 93d3362c..d6adc88c 100644 --- a/src/buildtool/build_engine/expression/TARGETS +++ b/src/buildtool/build_engine/expression/TARGETS @@ -4,8 +4,8 @@ , "hdrs": ["linked_map.hpp"] , "deps": [ ["@", "fmt", "", "fmt"] + , ["src/buildtool/multithreading", "atomic_value"] , ["src/utils/cpp", "hash_combine"] - , ["src/utils/cpp", "atomic"] ] , "stage": ["src", "buildtool", "build_engine", "expression"] } @@ -44,12 +44,12 @@ , ["src/buildtool/common", "artifact_description"] , ["src/buildtool/crypto", "hash_generator"] , ["src/buildtool/logging", "logging"] + , ["src/buildtool/multithreading", "atomic_value"] , ["src/utils/cpp", "type_safe_arithmetic"] , ["src/utils/cpp", "json"] , ["src/utils/cpp", "hash_combine"] , ["src/utils/cpp", "hex_string"] , ["src/utils/cpp", "concepts"] - , ["src/utils/cpp", "atomic"] , ["src/utils/cpp", "path"] , ["@", "gsl-lite", "", "gsl-lite"] ] diff --git a/src/buildtool/build_engine/expression/expression.cpp b/src/buildtool/build_engine/expression/expression.cpp index 679ebcbd..763f51f8 100644 --- a/src/buildtool/build_engine/expression/expression.cpp +++ b/src/buildtool/build_engine/expression/expression.cpp @@ -149,16 +149,7 @@ auto Expression::ToString() const -> std::string { // NOLINTNEXTLINE(misc-no-recursion) auto Expression::ToHash() const noexcept -> std::string { - if (hash_.load() == nullptr) { - if (not hash_loading_.exchange(true)) { - hash_ = std::make_shared(ComputeHash()); - hash_.notify_all(); - } - else { - hash_.wait(nullptr); - } - } - return *hash_.load(); + return hash_.SetOnceAndGet([this] { return ComputeHash(); }); } // NOLINTNEXTLINE(misc-no-recursion) diff --git a/src/buildtool/build_engine/expression/expression.hpp b/src/buildtool/build_engine/expression/expression.hpp index 412de057..6acb1e0e 100644 --- a/src/buildtool/build_engine/expression/expression.hpp +++ b/src/buildtool/build_engine/expression/expression.hpp @@ -20,7 +20,7 @@ #include "src/buildtool/build_engine/expression/target_result.hpp" #include "src/buildtool/common/artifact_description.hpp" #include "src/buildtool/crypto/hash_generator.hpp" -#include "src/utils/cpp/atomic.hpp" +#include "src/buildtool/multithreading/atomic_value.hpp" #include "src/utils/cpp/hex_string.hpp" #include "src/utils/cpp/json.hpp" @@ -63,22 +63,10 @@ class Expression { Expression() noexcept = default; ~Expression() noexcept = default; - Expression(Expression const& other) noexcept - : data_{other.data_}, hash_{other.hash_.load()} {} - Expression(Expression&& other) noexcept - : data_{std::move(other.data_)}, hash_{other.hash_.load()} {} - auto operator=(Expression const& other) noexcept -> Expression& { - if (this != &other) { - data_ = other.data_; - } - hash_ = other.hash_.load(); - return *this; - } - auto operator=(Expression&& other) noexcept -> Expression& { - data_ = std::move(other.data_); - hash_ = other.hash_.load(); - return *this; - } + Expression(Expression const& other) noexcept = default; + Expression(Expression&& other) noexcept = default; + auto operator=(Expression const& other) noexcept = delete; + auto operator=(Expression&& other) noexcept = delete; template requires(IsValidType>()) @@ -256,8 +244,7 @@ class Expression { map_t> data_{none_t{}}; - mutable atomic_shared_ptr hash_{}; - mutable std::atomic hash_loading_{}; + AtomicValue hash_{}; template requires(IsValidType()) [[nodiscard]] static consteval auto GetIndexOf() diff --git a/src/buildtool/build_engine/expression/linked_map.hpp b/src/buildtool/build_engine/expression/linked_map.hpp index 5c6da558..6266c2ce 100644 --- a/src/buildtool/build_engine/expression/linked_map.hpp +++ b/src/buildtool/build_engine/expression/linked_map.hpp @@ -10,6 +10,7 @@ #include #include "fmt/core.h" +#include "src/buildtool/multithreading/atomic_value.hpp" #include "src/utils/cpp/atomic.hpp" #include "src/utils/cpp/hash_combine.hpp" @@ -110,32 +111,12 @@ class LinkedMap { } LinkedMap() noexcept = default; - LinkedMap(LinkedMap const& other) noexcept - : next_{other.next_}, - content_{other.content_}, - map_{other.map_}, - items_{other.items_.load()} {} - LinkedMap(LinkedMap&& other) noexcept - : next_{std::move(other.next_)}, - content_{std::move(other.content_)}, - map_{std::move(other.map_)}, - items_{other.items_.load()} {} + LinkedMap(LinkedMap const& other) noexcept = default; + LinkedMap(LinkedMap&& other) noexcept = default; ~LinkedMap() noexcept = default; - auto operator=(LinkedMap const& other) noexcept -> LinkedMap& { - next_ = other.next_; - content_ = other.content_; - map_ = other.map_; - items_ = other.items_.load(); - return *this; - } - auto operator=(LinkedMap&& other) noexcept -> LinkedMap& { - next_ = std::move(other.next_); - content_ = std::move(other.content_); - map_ = std::move(other.map_); - items_ = other.items_.load(); - return *this; - } + auto operator=(LinkedMap const& other) noexcept = delete; + auto operator=(LinkedMap&& other) noexcept = delete; [[nodiscard]] auto contains(K const& key) const noexcept -> bool { return static_cast(Find(key)); @@ -276,23 +257,10 @@ class LinkedMap { // NOTE: Expensive, needs to compute sorted items. [[nodiscard]] auto Items() const& -> items_t const& { - if (items_.load() == nullptr) { - if (not items_loading_.exchange(true)) { - items_ = std::make_shared(ComputeSortedItems()); - items_.notify_all(); - } - else { - items_.wait(nullptr); - } - } - return *items_.load(); + return items_.SetOnceAndGet([this] { return ComputeSortedItems(); }); } - // NOTE: Expensive, needs to compute sorted items. - [[nodiscard]] auto Items() && -> items_t { - return items_.load() == nullptr ? ComputeSortedItems() - : std::move(*items_.load()); - } + [[nodiscard]] auto Items() && = delete; // NOTE: Expensive, needs to compute sorted items. [[nodiscard]] auto Keys() const -> keys_t { @@ -323,8 +291,7 @@ class LinkedMap { Ptr content_{}; // content of this map if set underlying_map_t map_{}; // content of this map if content_ is not set - mutable atomic_shared_ptr items_{}; - mutable std::atomic items_loading_{}; + AtomicValue items_{}; [[nodiscard]] auto ComputeSortedItems() const noexcept -> items_t { auto size = diff --git a/src/buildtool/file_system/TARGETS b/src/buildtool/file_system/TARGETS index 349c6d03..228cc3c9 100644 --- a/src/buildtool/file_system/TARGETS +++ b/src/buildtool/file_system/TARGETS @@ -48,7 +48,7 @@ , "object_type" , "file_system_manager" , ["src/buildtool/logging", "logging"] - , ["src/utils/cpp", "atomic"] + , ["src/buildtool/multithreading", "atomic_value"] , ["src/utils/cpp", "hex_string"] , ["@", "gsl-lite", "", "gsl-lite"] ] diff --git a/src/buildtool/file_system/git_tree.cpp b/src/buildtool/file_system/git_tree.cpp index 98281707..f4912a7b 100644 --- a/src/buildtool/file_system/git_tree.cpp +++ b/src/buildtool/file_system/git_tree.cpp @@ -13,9 +13,6 @@ namespace { constexpr auto kOIDRawSize{GIT_OID_RAWSZ}; -auto const kLoadTreeError = - std::make_shared>(std::nullopt); - [[nodiscard]] auto PermToType(std::string const& perm_str) noexcept -> std::optional { constexpr auto kPermBase = 8; @@ -154,26 +151,17 @@ auto GitTreeEntry::Blob() const noexcept -> std::optional { } auto GitTreeEntry::Tree() const& noexcept -> std::optional const& { - auto ptr = tree_cached_.load(); - if (not ptr) { - if (not tree_loading_.exchange(true)) { - ptr = kLoadTreeError; - std::optional obj{}; - if (IsTree() and (obj = cas_->ReadObject(raw_id_))) { + return tree_cached_.SetOnceAndGet([this]() -> std::optional { + std::optional obj{}; + if (IsTree()) { + if (auto obj = cas_->ReadObject(raw_id_)) { if (auto entries = ParseRawTreeObject(cas_, *obj)) { - ptr = std::make_shared>( - GitTree{cas_, std::move(*entries), raw_id_}); + return GitTree{cas_, std::move(*entries), raw_id_}; } } - tree_cached_.store(ptr); - tree_cached_.notify_all(); - } - else { - tree_cached_.wait(nullptr); - ptr = tree_cached_.load(); } - } - return *ptr; + return std::nullopt; + }); } auto GitTreeEntry::Size() const noexcept -> std::optional { diff --git a/src/buildtool/file_system/git_tree.hpp b/src/buildtool/file_system/git_tree.hpp index 8371d785..ff7dc828 100644 --- a/src/buildtool/file_system/git_tree.hpp +++ b/src/buildtool/file_system/git_tree.hpp @@ -8,7 +8,7 @@ #include "gsl-lite/gsl-lite.hpp" #include "src/buildtool/file_system/git_cas.hpp" #include "src/buildtool/file_system/object_type.hpp" -#include "src/utils/cpp/atomic.hpp" +#include "src/buildtool/multithreading/atomic_value.hpp" #include "src/utils/cpp/hex_string.hpp" class GitTreeEntry; @@ -85,8 +85,7 @@ class GitTreeEntry { gsl::not_null cas_; std::string raw_id_; ObjectType type_; - mutable atomic_shared_ptr> tree_cached_{nullptr}; - mutable std::atomic tree_loading_{false}; + AtomicValue> tree_cached_{}; }; using GitTreePtr = std::shared_ptr; diff --git a/src/buildtool/multithreading/TARGETS b/src/buildtool/multithreading/TARGETS index 88bed701..c1545f0f 100644 --- a/src/buildtool/multithreading/TARGETS +++ b/src/buildtool/multithreading/TARGETS @@ -52,4 +52,11 @@ ] , "stage": ["src", "buildtool", "multithreading"] } +, "atomic_value": + { "type": ["@", "rules", "CC", "library"] + , "name": ["atomic_value"] + , "hdrs": ["atomic_value.hpp"] + , "deps": [["src/utils/cpp", "atomic"]] + , "stage": ["src", "buildtool", "multithreading"] + } } diff --git a/src/buildtool/multithreading/atomic_value.hpp b/src/buildtool/multithreading/atomic_value.hpp new file mode 100644 index 00000000..2c108a96 --- /dev/null +++ b/src/buildtool/multithreading/atomic_value.hpp @@ -0,0 +1,54 @@ +#ifndef INCLUDED_SRC_BUILDTOOL_MULTITHREADING_ATOMIC_VALUE_HPP +#define INCLUDED_SRC_BUILDTOOL_MULTITHREADING_ATOMIC_VALUE_HPP + +#include +#include +#include + +#include "src/utils/cpp/atomic.hpp" + +// Value that can be set and get atomically. Reset is not thread-safe. +template +class AtomicValue { + public: + AtomicValue() noexcept = default; + AtomicValue(AtomicValue const& other) noexcept + : data_{other.data_.load()} {} + AtomicValue(AtomicValue&& other) noexcept : data_{other.data_.load()} {} + ~AtomicValue() noexcept = default; + + auto operator=(AtomicValue const& other) noexcept = delete; + auto operator=(AtomicValue&& other) noexcept = delete; + + // Atomically set value once and return its reference. If this method is + // called multiple times concurrently, the setter is called only once. In + // any case, this method blocks until the value is ready. + [[nodiscard]] auto SetOnceAndGet( + std::function const& setter) const& noexcept -> T const& { + if (data_.load() == nullptr) { + if (not load_.exchange(true)) { + data_.store(std::make_shared(setter())); + data_.notify_all(); + } + else { + data_.wait(nullptr); + } + } + return *data_.load(); + } + + [[nodiscard]] auto SetOnceAndGet(std::function const& setter) && = + delete; + + // Reset, not thread-safe! + void Reset() noexcept { + load_ = false; + data_ = nullptr; + } + + private: + mutable std::atomic load_{false}; + mutable atomic_shared_ptr data_{nullptr}; +}; + +#endif // INCLUDED_SRC_BUILDTOOL_MULTITHREADING_ATOMIC_VALUE_HPP -- cgit v1.2.3