From dc1db0e8b43f5e907a3ded2e39da8b58fa50a04b Mon Sep 17 00:00:00 2001 From: Maksim Denisov Date: Wed, 18 Sep 2024 17:51:41 +0200 Subject: Store HashFunction by const reference. Despite the fact that HashFunction is a small type, it still makes sense to store it by reference to reflect the ownership. StorageConfig becomes the main holder. Reference holders store HashFunction by const ref and aren't allowed to change it. However, they are free to return HashFunction by value since this doesn't benefit readability anyhow. --- src/buildtool/execution_api/common/api_bundle.cpp | 6 +++--- src/buildtool/execution_api/common/api_bundle.hpp | 4 +--- src/buildtool/execution_api/remote/bazel/bazel_api.cpp | 15 ++++++++------- src/buildtool/execution_api/remote/bazel/bazel_api.hpp | 2 +- .../execution_api/remote/bazel/bazel_network.cpp | 6 +++--- .../execution_api/remote/bazel/bazel_network.hpp | 17 +++++++++-------- .../execution_api/remote/bazel/bazel_network_reader.cpp | 4 ++-- .../execution_api/remote/bazel/bazel_network_reader.hpp | 9 +++++---- src/buildtool/execution_engine/executor/executor.hpp | 13 +++++++------ src/buildtool/file_system/object_cas.hpp | 6 +++--- src/buildtool/storage/local_cas.hpp | 8 ++++---- 11 files changed, 46 insertions(+), 44 deletions(-) (limited to 'src') diff --git a/src/buildtool/execution_api/common/api_bundle.cpp b/src/buildtool/execution_api/common/api_bundle.cpp index 8808e5d8..2106f555 100644 --- a/src/buildtool/execution_api/common/api_bundle.cpp +++ b/src/buildtool/execution_api/common/api_bundle.cpp @@ -24,7 +24,7 @@ auto ApiBundle::Create( gsl::not_null const& local_context, gsl::not_null const& remote_context, RepositoryConfig const* repo_config) -> ApiBundle { - auto const hash_fct = local_context->storage_config->hash_function; + auto const& hash_fct = local_context->storage_config->hash_function; IExecutionApi::Ptr local_api = std::make_shared(local_context, repo_config); IExecutionApi::Ptr remote_api = local_api; @@ -37,7 +37,7 @@ auto ApiBundle::Create( remote_context->auth, remote_context->retry_config, config, - hash_fct); + &hash_fct); } return ApiBundle{.hash_function = hash_fct, .local = std::move(local_api), @@ -58,7 +58,7 @@ auto ApiBundle::MakeRemote( authentication, retry_config, config, - hash_function); + &hash_function); } return local; } diff --git a/src/buildtool/execution_api/common/api_bundle.hpp b/src/buildtool/execution_api/common/api_bundle.hpp index 28595010..e342ce55 100644 --- a/src/buildtool/execution_api/common/api_bundle.hpp +++ b/src/buildtool/execution_api/common/api_bundle.hpp @@ -49,9 +49,7 @@ struct ApiBundle final { gsl::not_null const& retry_config) const -> gsl::not_null; - HashFunction const hash_function; - // 7 bytes of alignment. - + HashFunction const& hash_function; gsl::not_null const local; gsl::not_null const remote; }; diff --git a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp index e42c32e7..c5d77194 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp @@ -184,13 +184,14 @@ namespace { } // namespace -BazelApi::BazelApi(std::string const& instance_name, - std::string const& host, - Port port, - gsl::not_null const& auth, - gsl::not_null const& retry_config, - ExecutionConfiguration const& exec_config, - HashFunction hash_function) noexcept { +BazelApi::BazelApi( + std::string const& instance_name, + std::string const& host, + Port port, + gsl::not_null const& auth, + gsl::not_null const& retry_config, + ExecutionConfiguration const& exec_config, + gsl::not_null const& hash_function) noexcept { network_ = std::make_shared(instance_name, host, port, diff --git a/src/buildtool/execution_api/remote/bazel/bazel_api.hpp b/src/buildtool/execution_api/remote/bazel/bazel_api.hpp index 6b2a6f17..b829529c 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_api.hpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_api.hpp @@ -48,7 +48,7 @@ class BazelApi final : public IExecutionApi { gsl::not_null const& auth, gsl::not_null const& retry_config, ExecutionConfiguration const& exec_config, - HashFunction hash_function) noexcept; + gsl::not_null const& hash_function) noexcept; BazelApi(BazelApi const&) = delete; BazelApi(BazelApi&& other) noexcept; auto operator=(BazelApi const&) -> BazelApi& = delete; diff --git a/src/buildtool/execution_api/remote/bazel/bazel_network.cpp b/src/buildtool/execution_api/remote/bazel/bazel_network.cpp index edf18249..6cb7e905 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_network.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_network.cpp @@ -28,7 +28,7 @@ BazelNetwork::BazelNetwork( gsl::not_null const& auth, gsl::not_null const& retry_config, ExecutionConfiguration const& exec_config, - HashFunction hash_function) noexcept + gsl::not_null const& hash_function) noexcept : instance_name_{std::move(instance_name)}, cas_{std::make_unique(host, port, auth, retry_config)}, ac_{std::make_unique(host, port, auth, retry_config)}, @@ -37,7 +37,7 @@ BazelNetwork::BazelNetwork( auth, retry_config)}, exec_config_{exec_config}, - hash_function_{hash_function} {} + hash_function_{*hash_function} {} auto BazelNetwork::IsAvailable(bazel_re::Digest const& digest) const noexcept -> bool { @@ -144,7 +144,7 @@ auto BazelNetwork::ExecuteBazelActionSync( } auto BazelNetwork::CreateReader() const noexcept -> BazelNetworkReader { - return BazelNetworkReader{instance_name_, cas_.get(), hash_function_}; + return BazelNetworkReader{instance_name_, cas_.get(), &hash_function_}; } auto BazelNetwork::GetCachedActionResult( diff --git a/src/buildtool/execution_api/remote/bazel/bazel_network.hpp b/src/buildtool/execution_api/remote/bazel/bazel_network.hpp index 645b403d..a744daaf 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_network.hpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_network.hpp @@ -39,13 +39,14 @@ /// \brief Contains all network clients and is responsible for all network IO. class BazelNetwork { public: - explicit BazelNetwork(std::string instance_name, - std::string const& host, - Port port, - gsl::not_null const& auth, - gsl::not_null const& retry_config, - ExecutionConfiguration const& exec_config, - HashFunction hash_function) noexcept; + explicit BazelNetwork( + std::string instance_name, + std::string const& host, + Port port, + gsl::not_null const& auth, + gsl::not_null const& retry_config, + ExecutionConfiguration const& exec_config, + gsl::not_null const& hash_function) noexcept; /// \brief Check if digest exists in CAS /// \param[in] digest The digest to look up @@ -96,7 +97,7 @@ class BazelNetwork { std::unique_ptr ac_{}; std::unique_ptr exec_{}; ExecutionConfiguration exec_config_{}; - HashFunction const hash_function_; + HashFunction const& hash_function_; template [[nodiscard]] auto DoUploadBlobs(T_Iter const& first, diff --git a/src/buildtool/execution_api/remote/bazel/bazel_network_reader.cpp b/src/buildtool/execution_api/remote/bazel/bazel_network_reader.cpp index 5dac053b..d5f2f3f8 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_network_reader.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_network_reader.cpp @@ -30,10 +30,10 @@ BazelNetworkReader::BazelNetworkReader( std::string instance_name, gsl::not_null const& cas, - HashFunction hash_function) noexcept + gsl::not_null const& hash_function) noexcept : instance_name_{std::move(instance_name)}, cas_{*cas}, - hash_function_{hash_function} {} + hash_function_{*hash_function} {} BazelNetworkReader::BazelNetworkReader( BazelNetworkReader&& other, diff --git a/src/buildtool/execution_api/remote/bazel/bazel_network_reader.hpp b/src/buildtool/execution_api/remote/bazel/bazel_network_reader.hpp index 5c6cdbf9..e3e283c7 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_network_reader.hpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_network_reader.hpp @@ -41,9 +41,10 @@ class BazelNetworkReader final { public: using DumpCallback = std::function; - explicit BazelNetworkReader(std::string instance_name, - gsl::not_null const& cas, - HashFunction hash_function) noexcept; + explicit BazelNetworkReader( + std::string instance_name, + gsl::not_null const& cas, + gsl::not_null const& hash_function) noexcept; BazelNetworkReader( BazelNetworkReader&& other, @@ -84,7 +85,7 @@ class BazelNetworkReader final { std::string const instance_name_; BazelCasClient const& cas_; - HashFunction const hash_function_; + HashFunction const& hash_function_; std::optional auxiliary_map_; [[nodiscard]] auto MakeAuxiliaryMap( diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp index c9914ab6..9428a6e4 100644 --- a/src/buildtool/execution_engine/executor/executor.hpp +++ b/src/buildtool/execution_engine/executor/executor.hpp @@ -64,7 +64,7 @@ class ExecutorImpl { IExecutionApi const& api, ExecutionProperties const& merged_properties, gsl::not_null const& remote_context, - HashFunction hash_function, + gsl::not_null const& hash_function, std::chrono::milliseconds const& timeout, IExecutionAction::CacheFlag cache_flag, gsl::not_null const& stats, @@ -717,7 +717,8 @@ class ExecutorImpl { [[nodiscard]] static inline auto GetAlternativeEndpoint( const ExecutionProperties& properties, const gsl::not_null& remote_context, - HashFunction hash_function) -> std::unique_ptr { + const gsl::not_null& hash_function) + -> std::unique_ptr { for (auto const& [pred, endpoint] : remote_context->exec_config->dispatch) { bool match = true; @@ -783,7 +784,7 @@ class Executor { context_.remote_context->exec_config->platform_properties, action->ExecutionProperties()), context_.remote_context, - context_.apis->hash_function, + &context_.apis->hash_function, Impl::ScaleTime(timeout_, action->TimeoutScale()), action->NoCache() ? CF::DoNotCacheOutput : CF::CacheOutput, context_.statistics, @@ -806,7 +807,7 @@ class Executor { context_.remote_context->exec_config->platform_properties, action->ExecutionProperties()), context_.remote_context, - context_.apis->hash_function, + &context_.apis->hash_function, Impl::ScaleTime(timeout_, action->TimeoutScale()), action->NoCache() ? CF::DoNotCacheOutput : CF::CacheOutput, context_.statistics, @@ -880,7 +881,7 @@ class Rebuilder { context_.remote_context->exec_config->platform_properties, action->ExecutionProperties()), context_.remote_context, - context_.apis->hash_function, + &context_.apis->hash_function, Impl::ScaleTime(timeout_, action->TimeoutScale()), CF::PretendCached, context_.statistics, @@ -899,7 +900,7 @@ class Rebuilder { context_.remote_context->exec_config->platform_properties, action->ExecutionProperties()), context_.remote_context, - context_.apis->hash_function, + &context_.apis->hash_function, Impl::ScaleTime(timeout_, action->TimeoutScale()), CF::FromCacheOnly, context_.statistics, diff --git a/src/buildtool/file_system/object_cas.hpp b/src/buildtool/file_system/object_cas.hpp index edf43854..2616814c 100644 --- a/src/buildtool/file_system/object_cas.hpp +++ b/src/buildtool/file_system/object_cas.hpp @@ -53,13 +53,13 @@ class ObjectCAS { /// \param store_path The path to use for storing blobs. /// \param exists (optional) Function for checking blob existence. explicit ObjectCAS( - HashFunction hash_function, + gsl::not_null const& hash_function, std::filesystem::path const& store_path, std::optional> exists = std::nullopt) : file_store_{store_path}, exists_{exists.has_value() ? std::move(exists)->get() : kDefaultExists}, - hash_function_{hash_function} {} + hash_function_{*hash_function} {} ObjectCAS(ObjectCAS const&) = delete; ObjectCAS(ObjectCAS&&) = delete; @@ -115,7 +115,7 @@ class ObjectCAS { FileStorage file_store_; gsl::not_null exists_; - HashFunction const hash_function_; + HashFunction const& hash_function_; /// Default callback for checking blob existence. static inline ExistsFunc const kDefaultExists = [](auto const& /*digest*/, diff --git a/src/buildtool/storage/local_cas.hpp b/src/buildtool/storage/local_cas.hpp index 8a95c234..8c8d70c2 100644 --- a/src/buildtool/storage/local_cas.hpp +++ b/src/buildtool/storage/local_cas.hpp @@ -52,13 +52,13 @@ class LocalCAS { explicit LocalCAS( GenerationConfig const& config, gsl::not_null const*> const& uplinker) - : cas_file_{config.storage_config->hash_function, + : cas_file_{&config.storage_config->hash_function, config.cas_f, MakeUplinker(config, uplinker)}, - cas_exec_{config.storage_config->hash_function, + cas_exec_{&config.storage_config->hash_function, config.cas_x, MakeUplinker(config, uplinker)}, - cas_tree_{config.storage_config->hash_function, + cas_tree_{&config.storage_config->hash_function, config.cas_t, MakeUplinker(config, uplinker)}, cas_file_large_{this, config, uplinker}, @@ -284,7 +284,7 @@ class LocalCAS { ObjectCAS cas_tree_; LargeObjectCAS cas_file_large_; LargeObjectCAS cas_tree_large_; - HashFunction const hash_function_; + HashFunction const& hash_function_; /// \brief Provides uplink via "exists callback" for physical object CAS. template -- cgit v1.2.3