Avoid deep copies of containers in responses.

6 files changed, 90 insertions, 78 deletions

diff --git a/src/buildtool/execution_api/common/execution_response.hpp b/src/buildtool/execution_api/common/execution_response.hpp
index 3ad4e2ba..57e1f087 100644
--- a/src/buildtool/execution_api/common/execution_response.hpp
+++ b/src/buildtool/execution_api/common/execution_response.hpp
@@ -56,13 +56,12 @@ class IExecutionResponse {
 
     [[nodiscard]] virtual auto StdOut() noexcept -> std::string = 0;
 
-    [[nodiscard]] virtual auto ActionDigest() const noexcept -> std::string = 0;
+    [[nodiscard]] virtual auto ActionDigest() const noexcept
+        -> std::string const& = 0;
 
-    [[nodiscard]] virtual auto Artifacts() noexcept -> ArtifactInfos = 0;
-
-    // Artifacts plus extra useful info
-    [[nodiscard]] virtual auto ArtifactsWithDirSymlinks() noexcept
-        -> std::pair<ArtifactInfos, DirSymlinks> = 0;
+    [[nodiscard]] virtual auto Artifacts() noexcept -> ArtifactInfos const& = 0;
+    [[nodiscard]] virtual auto DirectorySymlinks() noexcept
+        -> DirSymlinks const& = 0;
 };
 
 #endif  // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_REMOTE_EXECUTION_RESPONSE_HPP
diff --git a/src/buildtool/execution_api/execution_service/execution_server.cpp b/src/buildtool/execution_api/execution_service/execution_server.cpp
index 132a812f..c99cfa04 100644
--- a/src/buildtool/execution_api/execution_service/execution_server.cpp
+++ b/src/buildtool/execution_api/execution_service/execution_server.cpp
@@ -281,15 +281,16 @@ static auto CreateTreeDigestFromDirectoryDigest(
 static auto AddOutputPaths(::bazel_re::ExecuteResponse* response,
                            IExecutionResponse::Ptr const& execution,
                            Storage const& storage) noexcept -> bool {
-    auto const& artifacts_plus = execution->ArtifactsWithDirSymlinks();
-    auto const& size = static_cast<int>(artifacts_plus.first.size());
+    auto const& artifacts = execution->Artifacts();
+    auto const& dir_symlinks = execution->DirectorySymlinks();
+    auto const size = static_cast<int>(artifacts.size());
     response->mutable_result()->mutable_output_files()->Reserve(size);
     response->mutable_result()->mutable_output_file_symlinks()->Reserve(size);
     response->mutable_result()->mutable_output_directory_symlinks()->Reserve(
         size);
     response->mutable_result()->mutable_output_directories()->Reserve(size);
 
-    for (auto const& [path, info] : artifacts_plus.first) {
+    for (auto const& [path, info] : artifacts) {
         auto dgst = static_cast<::bazel_re::Digest>(info.digest);
 
         if (info.type == ObjectType::Tree) {
@@ -326,7 +327,7 @@ static auto AddOutputPaths(::bazel_re::ExecuteResponse* response,
                 return false;
             }
             *(out_link.mutable_target()) = *content;
-            if (artifacts_plus.second.contains(path)) {
+            if (dir_symlinks.contains(path)) {
                 // directory symlink
                 response->mutable_result()
                     ->mutable_output_directory_symlinks()
diff --git a/src/buildtool/execution_api/local/local_response.hpp b/src/buildtool/execution_api/local/local_response.hpp
index 4871097a..21dd5952 100644
--- a/src/buildtool/execution_api/local/local_response.hpp
+++ b/src/buildtool/execution_api/local/local_response.hpp
@@ -67,31 +67,27 @@ class LocalResponse final : public IExecutionResponse {
     }
     auto IsCached() const noexcept -> bool final { return output_.is_cached; };
 
-    auto ActionDigest() const noexcept -> std::string final {
+    auto ActionDigest() const noexcept -> std::string const& final {
         return action_id_;
     }
 
-    auto Artifacts() noexcept -> ArtifactInfos final {
-        return ArtifactsWithDirSymlinks().first;
+    auto Artifacts() noexcept -> ArtifactInfos const& final {
+        Populate();
+        return artifacts_;
     }
 
-    auto ArtifactsWithDirSymlinks() noexcept
-        -> std::pair<ArtifactInfos, DirSymlinks> final {
-        // make sure to populate only once
-        populated_ = Populate();
-        if (not populated_) {
-            return {};
-        }
-        return std::pair(artifacts_, dir_symlinks_);
-    };
+    auto DirectorySymlinks() noexcept -> DirSymlinks const& final {
+        Populate();
+        return dir_symlinks_;
+    }
 
   private:
     std::string action_id_{};
     LocalAction::Output output_{};
     Storage const& storage_;
-    ArtifactInfos artifacts_{};
-    DirSymlinks dir_symlinks_{};
-    bool populated_{false};
+    ArtifactInfos artifacts_;
+    DirSymlinks dir_symlinks_;
+    bool populated_ = false;
 
     explicit LocalResponse(
         std::string action_id,
@@ -101,10 +97,13 @@ class LocalResponse final : public IExecutionResponse {
           output_{std::move(output)},
           storage_{*storage} {}
 
-    [[nodiscard]] auto Populate() noexcept -> bool {
+    void Populate() noexcept {
+        // Initialized only once lazily
         if (populated_) {
-            return true;
+            return;
         }
+        populated_ = true;
+
         ArtifactInfos artifacts{};
         auto const& action_result = output_.action;
         artifacts.reserve(
@@ -116,7 +115,7 @@ class LocalResponse final : public IExecutionResponse {
             static_cast<std::size_t>(action_result.output_directories_size()));
 
         DirSymlinks dir_symlinks{};
-        dir_symlinks_.reserve(static_cast<std::size_t>(
+        dir_symlinks.reserve(static_cast<std::size_t>(
             action_result.output_directory_symlinks_size()));
 
         // collect files and store them
@@ -129,7 +128,7 @@ class LocalResponse final : public IExecutionResponse {
                         .type = file.is_executable() ? ObjectType::Executable
                                                      : ObjectType::File});
             } catch (...) {
-                return false;
+                return;
             }
         }
 
@@ -143,7 +142,7 @@ class LocalResponse final : public IExecutionResponse {
                             storage_.GetHashFunction(), link.target()),
                         .type = ObjectType::Symlink});
             } catch (...) {
-                return false;
+                return;
             }
         }
         for (auto const& link : action_result.output_directory_symlinks()) {
@@ -156,7 +155,7 @@ class LocalResponse final : public IExecutionResponse {
                         .type = ObjectType::Symlink});
                 dir_symlinks.emplace(link.path());  // add it to set
             } catch (...) {
-                return false;
+                return;
             }
         }
 
@@ -169,12 +168,11 @@ class LocalResponse final : public IExecutionResponse {
                         .digest = ArtifactDigest{dir.tree_digest()},
                         .type = ObjectType::Tree});
             } catch (...) {
-                return false;
+                return;
             }
         }
         artifacts_ = std::move(artifacts);
         dir_symlinks_ = std::move(dir_symlinks);
-        return true;
     }
 };
 
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
index ae9f00e3..914836fe 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
@@ -50,24 +50,23 @@ auto BazelResponse::ReadStringBlob(bazel_re::Digest const& id) noexcept
     return std::string{};
 }
 
-auto BazelResponse::Artifacts() noexcept -> ArtifactInfos {
-    return ArtifactsWithDirSymlinks().first;
+auto BazelResponse::Artifacts() noexcept -> ArtifactInfos const& {
+    Populate();
+    return artifacts_;
 }
 
-auto BazelResponse::ArtifactsWithDirSymlinks() noexcept
-    -> std::pair<ArtifactInfos, DirSymlinks> {
-    // make sure to populate only once
-    populated_ = Populate();
-    if (not populated_) {
-        return {};
-    }
-    return std::pair(artifacts_, dir_symlinks_);
+auto BazelResponse::DirectorySymlinks() noexcept -> DirSymlinks const& {
+    Populate();
+    return dir_symlinks_;
 }
 
-auto BazelResponse::Populate() noexcept -> bool {
+void BazelResponse::Populate() noexcept {
+    // Initialized only once lazily
     if (populated_) {
-        return true;
+        return;
     }
+    populated_ = true;
+
     ArtifactInfos artifacts{};
     auto const& action_result = output_.action_result;
     artifacts.reserve(
@@ -91,7 +90,7 @@ auto BazelResponse::Populate() noexcept -> bool {
                                                  ? ObjectType::Executable
                                                  : ObjectType::File});
         } catch (...) {
-            return false;
+            return;
         }
     }
 
@@ -105,7 +104,7 @@ auto BazelResponse::Populate() noexcept -> bool {
                         network_->GetHashFunction(), link.target()),
                     .type = ObjectType::Symlink});
         } catch (...) {
-            return false;
+            return;
         }
     }
     for (auto const& link : action_result.output_directory_symlinks()) {
@@ -118,7 +117,7 @@ auto BazelResponse::Populate() noexcept -> bool {
                     .type = ObjectType::Symlink});
             dir_symlinks.emplace(link.path());  // add it to set
         } catch (...) {
-            return false;
+            return;
         }
     }
 
@@ -133,12 +132,12 @@ auto BazelResponse::Populate() noexcept -> bool {
                         .digest = ArtifactDigest{tree.tree_digest()},
                         .type = ObjectType::Tree});
             } catch (...) {
-                return false;
+                return;
             }
         }
         artifacts_ = std::move(artifacts);
         dir_symlinks_ = std::move(dir_symlinks);
-        return true;
+        return;
     }
 
     // obtain tree digests for output directories
@@ -159,7 +158,7 @@ auto BazelResponse::Populate() noexcept -> bool {
                 auto tree = BazelMsgFactory::MessageFromString<bazel_re::Tree>(
                     *tree_blob.data);
                 if (not tree) {
-                    return false;
+                    return;
                 }
 
                 // The server does not store the Directory messages it just
@@ -168,21 +167,20 @@ auto BazelResponse::Populate() noexcept -> bool {
                 // have to upload them manually.
                 auto root_digest = UploadTreeMessageDirectories(*tree);
                 if (not root_digest) {
-                    return false;
+                    return;
                 }
                 artifacts.emplace(
                     action_result.output_directories(pos).path(),
                     Artifact::ObjectInfo{.digest = *root_digest,
                                          .type = ObjectType::Tree});
             } catch (...) {
-                return false;
+                return;
             }
             ++pos;
         }
     }
     artifacts_ = std::move(artifacts);
     dir_symlinks_ = std::move(dir_symlinks);
-    return true;
 }
 
 auto BazelResponse::UploadTreeMessageDirectories(
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
index 30a70d7f..6e37d344 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
@@ -15,6 +15,7 @@
 #ifndef INCLUDED_SRC_BUILDTOOL_EXECUTION_API_REMOTE_BAZEL_BAZEL_RESPONSE_HPP
 #define INCLUDED_SRC_BUILDTOOL_EXECUTION_API_REMOTE_BAZEL_BAZEL_RESPONSE_HPP
 
+#include <optional>
 #include <string>
 #include <utility>  // std::move
 #include <vector>
@@ -53,22 +54,20 @@ class BazelResponse final : public IExecutionResponse {
         return output_.cached_result;
     };
 
-    auto ActionDigest() const noexcept -> std::string final {
+    auto ActionDigest() const noexcept -> std::string const& final {
         return action_id_;
     }
 
-    auto Artifacts() noexcept -> ArtifactInfos final;
-
-    auto ArtifactsWithDirSymlinks() noexcept
-        -> std::pair<ArtifactInfos, DirSymlinks> final;
+    auto Artifacts() noexcept -> ArtifactInfos const& final;
+    auto DirectorySymlinks() noexcept -> DirSymlinks const& final;
 
   private:
     std::string action_id_{};
     std::shared_ptr<BazelNetwork> const network_{};
     BazelExecutionClient::ExecutionOutput output_{};
-    ArtifactInfos artifacts_{};
-    DirSymlinks dir_symlinks_{};
-    bool populated_{false};
+    ArtifactInfos artifacts_;
+    DirSymlinks dir_symlinks_;
+    bool populated_ = false;
 
     explicit BazelResponse(std::string action_id,
                            std::shared_ptr<BazelNetwork> network,
@@ -85,7 +84,7 @@ class BazelResponse final : public IExecutionResponse {
         return id.size_bytes() != 0;
     }
 
-    [[nodiscard]] auto Populate() noexcept -> bool;
+    void Populate() noexcept;
 
     [[nodiscard]] auto UploadTreeMessageDirectories(
         bazel_re::Tree const& tree) const -> std::optional<ArtifactDigest>;
diff --git a/test/buildtool/execution_engine/executor/executor.test.cpp b/test/buildtool/execution_engine/executor/executor.test.cpp
index 6a32a497..84bf7ade 100644
--- a/test/buildtool/execution_engine/executor/executor.test.cpp
+++ b/test/buildtool/execution_engine/executor/executor.test.cpp
@@ -83,10 +83,37 @@ class TestResponse : public IExecutionResponse {
     [[nodiscard]] auto HasStdOut() const noexcept -> bool final { return true; }
     [[nodiscard]] auto StdErr() noexcept -> std::string final { return {}; }
     [[nodiscard]] auto StdOut() noexcept -> std::string final { return {}; }
-    [[nodiscard]] auto ActionDigest() const noexcept -> std::string final {
-        return {};
+    [[nodiscard]] auto ActionDigest() const noexcept
+        -> std::string const& final {
+        static const std::string kEmptyHash;
+        return kEmptyHash;
     }
-    [[nodiscard]] auto Artifacts() noexcept -> ArtifactInfos final {
+    [[nodiscard]] auto Artifacts() noexcept -> ArtifactInfos const& final {
+        if (not populated_) {
+            Populate();
+        }
+        return artifacts_;
+    }
+    [[nodiscard]] auto DirectorySymlinks() noexcept
+        -> DirSymlinks const& final {
+        static const DirSymlinks kEmptySymlinks{};
+        return kEmptySymlinks;
+    }
+
+  private:
+    TestApiConfig config_{};
+    ArtifactInfos artifacts_;
+    bool populated_ = false;
+
+    explicit TestResponse(TestApiConfig config) noexcept
+        : config_{std::move(config)} {}
+
+    void Populate() noexcept {
+        if (populated_) {
+            return;
+        }
+        populated_ = true;
+
         ArtifactInfos artifacts{};
         artifacts.reserve(config_.execution.outputs.size());
 
@@ -99,21 +126,11 @@ class TestResponse : public IExecutionResponse {
                         .digest = ArtifactDigest{path, 0, /*is_tree=*/false},
                         .type = ObjectType::File});
             } catch (...) {
-                return {};
+                return;
             }
         }
-
-        return artifacts;
+        artifacts_ = std::move(artifacts);
     }
-    [[nodiscard]] auto ArtifactsWithDirSymlinks() noexcept
-        -> std::pair<ArtifactInfos, DirSymlinks> final {
-        return {};
-    }
-
-  private:
-    TestApiConfig config_{};
-    explicit TestResponse(TestApiConfig config) noexcept
-        : config_{std::move(config)} {}
 };
 
 /// \brief Mockup Action, stores only config