From ea2291d24d531a1ea221f1035636303ac0da787d Mon Sep 17 00:00:00 2001 From: Maksim Denisov Date: Tue, 28 Jan 2025 10:13:50 +0100 Subject: Replace ContentBlobContainer with std::unordered_set --- .../bazel_msg/bazel_blob_container.hpp | 4 ++- .../common/artifact_blob_container.hpp | 4 ++- src/buildtool/execution_api/common/common_api.hpp | 38 ++++++++++++++-------- src/buildtool/execution_api/local/TARGETS | 1 - src/buildtool/execution_api/local/local_api.hpp | 6 ++-- src/buildtool/execution_api/remote/TARGETS | 2 -- .../execution_api/remote/bazel/bazel_api.cpp | 5 ++- .../execution_api/remote/bazel/bazel_response.cpp | 8 +---- 8 files changed, 35 insertions(+), 33 deletions(-) (limited to 'src') diff --git a/src/buildtool/execution_api/bazel_msg/bazel_blob_container.hpp b/src/buildtool/execution_api/bazel_msg/bazel_blob_container.hpp index 039ddf9c..9ac738d6 100644 --- a/src/buildtool/execution_api/bazel_msg/bazel_blob_container.hpp +++ b/src/buildtool/execution_api/bazel_msg/bazel_blob_container.hpp @@ -15,10 +15,12 @@ #ifndef INCLUDED_SRC_BUILDTOOL_EXECUTION_API_BAZEL_MSG_BAZEL_BLOB_CONTAINER_HPP #define INCLUDED_SRC_BUILDTOOL_EXECUTION_API_BAZEL_MSG_BAZEL_BLOB_CONTAINER_HPP +#include + #include "src/buildtool/common/bazel_types.hpp" #include "src/buildtool/execution_api/common/content_blob_container.hpp" using BazelBlob = ContentBlob; -using BazelBlobContainer = ContentBlobContainer; +using BazelBlobContainer = std::unordered_set; #endif // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_BAZEL_MSG_BAZEL_BLOB_CONTAINER_HPP diff --git a/src/buildtool/execution_api/common/artifact_blob_container.hpp b/src/buildtool/execution_api/common/artifact_blob_container.hpp index 32f3fc38..76b0692e 100644 --- a/src/buildtool/execution_api/common/artifact_blob_container.hpp +++ b/src/buildtool/execution_api/common/artifact_blob_container.hpp @@ -15,10 +15,12 @@ #ifndef INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_ARTIFACT_BLOB_CONTAINER_HPP #define INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_ARTIFACT_BLOB_CONTAINER_HPP +#include + #include "src/buildtool/common/artifact_digest.hpp" #include "src/buildtool/execution_api/common/content_blob_container.hpp" using ArtifactBlob = ContentBlob; -using ArtifactBlobContainer = ContentBlobContainer; +using ArtifactBlobContainer = std::unordered_set; #endif // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_ARTIFACT_BLOB_CONTAINER_HPP diff --git a/src/buildtool/execution_api/common/common_api.hpp b/src/buildtool/execution_api/common/common_api.hpp index f58e4893..5060c086 100644 --- a/src/buildtool/execution_api/common/common_api.hpp +++ b/src/buildtool/execution_api/common/common_api.hpp @@ -15,6 +15,7 @@ #ifndef INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_COMMON_API_HPP #define INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_COMMON_API_HPP +#include #include #include #include @@ -122,10 +123,11 @@ template /// \returns Returns true on success, false otherwise (failures or exceptions). template auto UpdateContainerAndUpload( - gsl::not_null*> const& container, + gsl::not_null>*> const& container, ContentBlob&& blob, bool exception_is_fatal, - std::function&&)> const& uploader, + std::function>&&)> const& + uploader, Logger const* logger = nullptr) noexcept -> bool { // Optimize upload of blobs with respect to the maximum transfer limit, such // that we never store unnecessarily more data in the container than we need @@ -133,24 +135,32 @@ auto UpdateContainerAndUpload( try { if (blob.data->size() > kMaxBatchTransferSize) { // large blobs use individual stream upload - if (not uploader(ContentBlobContainer{{blob}})) { + if (not uploader(std::unordered_set>{ + {std::move(blob)}})) { return false; } } else { - if (container->ContentSize() + blob.data->size() > - kMaxBatchTransferSize) { - // swap away from original container to allow move during upload - ContentBlobContainer tmp_container{}; - std::swap(*container, tmp_container); - // if we would surpass the transfer limit, upload the current - // container and clear it before adding more blobs - if (not uploader(std::move(tmp_container))) { - return false; + if (not container->contains(blob)) { + std::size_t content_size = 0; + for (auto const& blob : *container) { + content_size += blob.data->size(); } + + if (content_size + blob.data->size() > kMaxBatchTransferSize) { + // swap away from original container to allow move during + // upload + std::unordered_set> tmp_container{}; + std::swap(*container, tmp_container); + // if we would surpass the transfer limit, upload the + // current container and clear it before adding more blobs + if (not uploader(std::move(tmp_container))) { + return false; + } + } + // add current blob to container + container->emplace(std::move(blob)); } - // add current blob to container - container->Emplace(std::move(blob)); } } catch (std::exception const& ex) { if (exception_is_fatal) { diff --git a/src/buildtool/execution_api/local/TARGETS b/src/buildtool/execution_api/local/TARGETS index b9d839a9..a8fd90f0 100644 --- a/src/buildtool/execution_api/local/TARGETS +++ b/src/buildtool/execution_api/local/TARGETS @@ -44,7 +44,6 @@ , ["src/utils/cpp", "expected"] , ["src/utils/cpp", "path"] , ["src/utils/cpp", "tmp_dir"] - , ["src/utils/cpp", "transformed_range"] ] , "stage": ["src", "buildtool", "execution_api", "local"] , "private-deps": diff --git a/src/buildtool/execution_api/local/local_api.hpp b/src/buildtool/execution_api/local/local_api.hpp index a9d6b660..07bef457 100644 --- a/src/buildtool/execution_api/local/local_api.hpp +++ b/src/buildtool/execution_api/local/local_api.hpp @@ -60,7 +60,6 @@ #include "src/buildtool/storage/config.hpp" #include "src/buildtool/storage/storage.hpp" #include "src/utils/cpp/expected.hpp" -#include "src/utils/cpp/transformed_range.hpp" /// \brief API for local execution. class LocalApi final : public IExecutionApi { @@ -250,10 +249,9 @@ class LocalApi final : public IExecutionApi { [[nodiscard]] auto Upload(ArtifactBlobContainer&& blobs, bool /*skip_find_missing*/) const noexcept -> bool final { - auto const range = blobs.Blobs(); return std::all_of( - range.begin(), - range.end(), + blobs.begin(), + blobs.end(), [&cas = local_context_.storage->CAS()](ArtifactBlob const& blob) { auto const cas_digest = blob.digest.IsTree() diff --git a/src/buildtool/execution_api/remote/TARGETS b/src/buildtool/execution_api/remote/TARGETS index 1470d62a..1a7a15af 100644 --- a/src/buildtool/execution_api/remote/TARGETS +++ b/src/buildtool/execution_api/remote/TARGETS @@ -63,7 +63,6 @@ , ["src/utils/cpp", "back_map"] , ["src/utils/cpp", "gsl"] , ["src/utils/cpp", "path"] - , ["src/utils/cpp", "transformed_range"] ] } , "bazel": @@ -99,7 +98,6 @@ , ["src/buildtool/multithreading", "task_system"] , ["src/utils/cpp", "back_map"] , ["src/utils/cpp", "expected"] - , ["src/utils/cpp", "transformed_range"] ] } , "config": diff --git a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp index 1c8fd96d..9c3de203 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp @@ -49,7 +49,6 @@ #include "src/buildtool/multithreading/task_system.hpp" #include "src/utils/cpp/back_map.hpp" #include "src/utils/cpp/expected.hpp" -#include "src/utils/cpp/transformed_range.hpp" namespace { @@ -144,8 +143,8 @@ namespace { -> std::optional> { std::unordered_set blobs; try { - blobs.reserve(container.Size()); - for (const auto& blob : container.Blobs()) { + blobs.reserve(container.size()); + for (const auto& blob : container) { blobs.emplace(ArtifactDigestFactory::ToBazel(blob.digest), blob.data, blob.is_exec); diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp index 2a06985d..3aaecb88 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp @@ -20,7 +20,6 @@ #include #include #include -#include #include #include "fmt/core.h" @@ -41,7 +40,6 @@ #include "src/buildtool/logging/logger.hpp" #include "src/utils/cpp/gsl.hpp" #include "src/utils/cpp/path.hpp" -#include "src/utils/cpp/transformed_range.hpp" namespace { @@ -278,11 +276,7 @@ auto BazelResponse::UploadTreeMessageDirectories( bazel_re::Tree const& tree) const -> expected { auto const upload_callback = [&network = *network_](BazelBlobContainer&& blobs) -> bool { - std::unordered_set bazel_blobs; - for (auto const& blob : blobs.Blobs()) { - bazel_blobs.emplace(blob); - } - return network.UploadBlobs(std::move(bazel_blobs)); + return network.UploadBlobs(std::move(blobs)); }; auto const hash_function = network_->GetHashFunction(); BazelBlobContainer dir_blobs{}; -- cgit v1.2.3