Absent target: deduplicate serve calls asking for flexible variables

For an absent export target, the first step of analysis is to ask
serve for the flexible variables. The answer to this request is,
however, independent of the configuration for this target. So we
can avoid calls by caching the answer in an additional map.

4 files changed, 261 insertions, 153 deletions

diff --git a/src/buildtool/build_engine/target_map/TARGETS b/src/buildtool/build_engine/target_map/TARGETS
index 70d99908..97576c2c 100644
--- a/src/buildtool/build_engine/target_map/TARGETS
+++ b/src/buildtool/build_engine/target_map/TARGETS
@@ -104,6 +104,7 @@
     , ["src/buildtool/common", "config"]
     , ["src/buildtool/multithreading", "async_map_consumer"]
     , ["src/buildtool/progress_reporting", "progress"]
+    , ["src/utils/cpp", "hash_combine"]
     , ["@", "gsl", "", "gsl"]
     ]
   , "stage": ["src", "buildtool", "build_engine", "target_map"]
diff --git a/src/buildtool/build_engine/target_map/absent_target_map.cpp b/src/buildtool/build_engine/target_map/absent_target_map.cpp
index 7b67ced8..4657e002 100644
--- a/src/buildtool/build_engine/target_map/absent_target_map.cpp
+++ b/src/buildtool/build_engine/target_map/absent_target_map.cpp
@@ -20,175 +20,235 @@
 #include "src/buildtool/storage/target_cache_key.hpp"
 #endif
 
-auto BuildMaps::Target::CreateAbsentTargetMap(
-    const gsl::not_null<ResultTargetMap*>& result_map,
+#ifndef BOOTSTRAP_BUILD_TOOL
+
+namespace {
+void WithFlexibleVariables(
+    const BuildMaps::Target::ConfiguredTarget& key,
+    std::vector<std::string> flexible_vars,
     gsl::not_null<RepositoryConfig*> const& repo_config,
+    const BuildMaps::Target::AbsentTargetMap::SubCallerPtr& subcaller,
+    const BuildMaps::Target::AbsentTargetMap::SetterPtr& setter,
+    const BuildMaps::Target::AbsentTargetMap::LoggerPtr& logger,
+    const gsl::not_null<BuildMaps::Target::ResultTargetMap*> result_map,
     gsl::not_null<Statistics*> const& stats,
-    gsl::not_null<Progress*> const& exports_progress,
-    std::size_t jobs) -> AbsentTargetMap {
-#ifndef BOOTSTRAP_BUILD_TOOL
-    auto target_reader = [result_map, repo_config, stats, exports_progress](
-                             auto /*ts*/,
-                             auto setter,
-                             auto logger,
-                             auto subcaller,
-                             auto key) {
-        // assumptions:
-        // - target with absent targets file requested
-        // - ServeApi correctly configured
-        auto const& repo_name = key.target.ToModule().repository;
-        auto target_root_id =
-            repo_config->TargetRoot(repo_name)->GetAbsentTreeId();
-        if (!target_root_id) {
-            (*logger)(fmt::format("Failed to get the target root id for "
-                                  "repository \"{}\"",
-                                  repo_name),
-                      /*fatal=*/true);
-            return;
-        }
-        auto flexible_vars = ServeApi::ServeTargetVariables(
-            *target_root_id,
-            *(repo_config->TargetFileName(repo_name)),
-            key.target.GetNamedTarget().name);
-        if (!flexible_vars) {
-            (*logger)(fmt::format("Failed to obtain flexible config variables "
-                                  "for absent target {}",
+    gsl::not_null<Progress*> const& exports_progress) {
+    auto effective_config = key.config.Prune(flexible_vars);
+    if (key.config != effective_config) {
+        (*subcaller)(
+            {BuildMaps::Target::ConfiguredTarget{.target = key.target,
+                                                 .config = effective_config}},
+            [setter](auto const& values) {
+                AnalysedTargetPtr result = *(values[0]);
+                (*setter)(std::move(result));
+            },
+            logger);
+        return;
+    }
+
+    // TODO(asartori): avoid code duplication in export.cpp
+    stats->IncrementExportsFoundCounter();
+    auto target_name = key.target.GetNamedTarget();
+    auto repo_key = repo_config->RepositoryKey(target_name.repository);
+    if (!repo_key) {
+        (*logger)(fmt::format("Failed to obtain repository key for repo \"{}\"",
+                              target_name.repository),
+                  /*fatal=*/true);
+        return;
+    }
+    auto target_cache_key =
+        TargetCacheKey::Create(*repo_key, target_name, effective_config);
+    if (not target_cache_key) {
+        (*logger)(fmt::format("Could not produce cache key for target {}",
+                              key.target.ToString()),
+                  /*fatal=*/true);
+        return;
+    }
+    std::optional<std::pair<TargetCacheEntry, Artifact::ObjectInfo>>
+        target_cache_value{std::nullopt};
+    target_cache_value =
+        Storage::Instance().TargetCache().Read(*target_cache_key);
+    bool from_just_serve = false;
+    if (!target_cache_value) {
+        Logger::Log(LogLevel::Debug,
+                    "Querying serve endpoint for absent export target {}",
+                    key.target.ToString());
+        exports_progress->TaskTracker().Start(
+            target_cache_key->Id().ToString());
+        auto res = ServeApi::ServeTarget(*target_cache_key, *repo_key);
+        // process response from serve endpoint
+        if (not res) {
+            // report target not found
+            (*logger)(fmt::format("Absent target {} was not found on serve "
+                                  "endpoint",
                                   key.target.ToString()),
                       /*fatal=*/true);
             return;
         }
-        auto effective_config = key.config.Prune(*flexible_vars);
-        if (key.config != effective_config) {
-            (*subcaller)(
-                {BuildMaps::Target::ConfiguredTarget{
-                    .target = key.target, .config = effective_config}},
-                [setter](auto const& values) {
-                    AnalysedTargetPtr result = *(values[0]);
-                    (*setter)(std::move(result));
-                },
-                logger);
-            return;
-        }
-
-        // we know now that this target is an export target
-        // TODO(asartori): avoid code duplication in export.cpp
-        stats->IncrementExportsFoundCounter();
-        auto target_name = key.target.GetNamedTarget();
-        auto repo_key = repo_config->RepositoryKey(target_name.repository);
-        if (!repo_key) {
-            (*logger)(
-                fmt::format("Failed to obtain repository key for repo \"{}\"",
-                            target_name.repository),
-                /*fatal=*/true);
-            return;
-        }
-        auto target_cache_key =
-            TargetCacheKey::Create(*repo_key, target_name, effective_config);
-        if (not target_cache_key) {
-            (*logger)(fmt::format("Could not produce cache key for target {}",
-                                  key.target.ToString()),
+        if (res->index() == 0) {
+            (*logger)(fmt::format("Failure to remotely analyse or build absent "
+                                  "target {}\nDetailed log available on the "
+                                  "remote-execution endpoint as blob {}",
+                                  key.target.ToString(),
+                                  std::get<0>(*res)),
                       /*fatal=*/true);
             return;
         }
-        std::optional<std::pair<TargetCacheEntry, Artifact::ObjectInfo>>
-            target_cache_value{std::nullopt};
-        target_cache_value =
-            Storage::Instance().TargetCache().Read(*target_cache_key);
-        bool from_just_serve = false;
-        if (!target_cache_value) {
-            Logger::Log(LogLevel::Debug,
-                        "Querying serve endpoint for absent export target {}",
-                        key.target.ToString());
-            exports_progress->TaskTracker().Start(
-                target_cache_key->Id().ToString());
-            auto res = ServeApi::ServeTarget(*target_cache_key, *repo_key);
-            // process response from serve endpoint
-            if (not res) {
-                // report target not found
-                (*logger)(fmt::format("Absent target {} was not found on serve "
-                                      "endpoint",
-                                      key.target.ToString()),
-                          /*fatal=*/true);
-                return;
-            }
-            if (res->index() == 0) {
-                (*logger)(
-                    fmt::format("Failure to remotely analyse or build absent "
-                                "target {}\nDetailed log available on the "
-                                "remote-execution endpoint as blob {}",
-                                key.target.ToString(),
-                                std::get<0>(*res)),
-                    /*fatal=*/true);
-                return;
-            }
-            if (res->index() == 1) {
-                (*logger)(fmt::format("While querying serve endpoint for "
-                                      "absent export target {}:\n{}",
-                                      key.target.ToString(),
-                                      std::get<1>(*res)),
-                          /*fatal=*/true);
-                return;
-            }
-            // index == 2
-            target_cache_value = std::get<2>(*res);
-            exports_progress->TaskTracker().Stop(
-                target_cache_key->Id().ToString());
-            from_just_serve = true;
-        }
-
-        if (!target_cache_value) {
-            (*logger)(fmt::format("Could not get target cache value for key {}",
-                                  target_cache_key->Id().ToString()),
+        if (res->index() == 1) {
+            (*logger)(fmt::format("While querying serve endpoint for "
+                                  "absent export target {}:\n{}",
+                                  key.target.ToString(),
+                                  std::get<1>(*res)),
                       /*fatal=*/true);
             return;
         }
-        auto const& [entry, info] = *target_cache_value;
-        if (auto result = entry.ToResult()) {
-            auto deps_info = TargetGraphInformation{
-                std::make_shared<BuildMaps::Target::ConfiguredTarget>(
-                    BuildMaps::Target::ConfiguredTarget{
-                        .target = key.target, .config = effective_config}),
-                {},
-                {},
-                {}};
+        // index == 2
+        target_cache_value = std::get<2>(*res);
+        exports_progress->TaskTracker().Stop(target_cache_key->Id().ToString());
+        from_just_serve = true;
+    }
 
-            auto analysis_result = std::make_shared<AnalysedTarget const>(
-                *result,
-                std::vector<ActionDescription::Ptr>{},
-                std::vector<std::string>{},
-                std::vector<Tree::Ptr>{},
-                std::unordered_set<std::string>{flexible_vars->begin(),
-                                                flexible_vars->end()},
-                std::set<std::string>{},
-                entry.ToImplied(),
-                deps_info);
+    if (!target_cache_value) {
+        (*logger)(fmt::format("Could not get target cache value for key {}",
+                              target_cache_key->Id().ToString()),
+                  /*fatal=*/true);
+        return;
+    }
+    auto const& [entry, info] = *target_cache_value;
+    if (auto result = entry.ToResult()) {
+        auto deps_info = TargetGraphInformation{
+            std::make_shared<BuildMaps::Target::ConfiguredTarget>(
+                BuildMaps::Target::ConfiguredTarget{
+                    .target = key.target, .config = effective_config}),
+            {},
+            {},
+            {}};
 
-            analysis_result = result_map->Add(key.target,
-                                              effective_config,
-                                              analysis_result,
-                                              std::nullopt,
-                                              true);
+        auto analysis_result = std::make_shared<AnalysedTarget const>(
+            *result,
+            std::vector<ActionDescription::Ptr>{},
+            std::vector<std::string>{},
+            std::vector<Tree::Ptr>{},
+            std::unordered_set<std::string>{flexible_vars.begin(),
+                                            flexible_vars.end()},
+            std::set<std::string>{},
+            entry.ToImplied(),
+            deps_info);
 
-            Logger::Log(LogLevel::Performance,
-                        "Absent export target {} taken from {}: {} -> {}",
-                        key.target.ToString(),
-                        (from_just_serve ? "serve endpoint" : "cache"),
-                        target_cache_key->Id().ToString(),
-                        info.ToString());
+        analysis_result = result_map->Add(
+            key.target, effective_config, analysis_result, std::nullopt, true);
 
-            (*setter)(std::move(analysis_result));
-            if (from_just_serve) {
-                stats->IncrementExportsServedCounter();
-            }
-            else {
-                stats->IncrementExportsCachedCounter();
-            }
+        Logger::Log(LogLevel::Performance,
+                    "Absent export target {} taken from {}: {} -> {}",
+                    key.target.ToString(),
+                    (from_just_serve ? "serve endpoint" : "cache"),
+                    target_cache_key->Id().ToString(),
+                    info.ToString());
+
+        (*setter)(std::move(analysis_result));
+        if (from_just_serve) {
+            stats->IncrementExportsServedCounter();
+        }
+        else {
+            stats->IncrementExportsCachedCounter();
+        }
+        return;
+    }
+    (*logger)(fmt::format("Reading target entry for key {} failed",
+                          target_cache_key->Id().ToString()),
+              /*fatal=*/true);
+}
+
+}  // namespace
+
+#endif
+
+auto BuildMaps::Target::CreateAbsentTargetVariablesMap(std::size_t jobs)
+    -> AbsentTargetVariablesMap {
+#ifdef BOOTSTRAP_BUILD_TOOL
+    auto target_variables = [](auto /*ts*/,
+                               auto /*setter*/,
+                               auto /*logger*/,
+                               auto /*subcaller*/,
+                               auto /*key*/) {};
+#else
+    auto target_variables = [](auto /*ts*/,
+                               auto setter,
+                               auto logger,
+                               auto /*subcaller*/,
+                               auto key) {
+        auto flexible_vars_opt = ServeApi::ServeTargetVariables(
+            key.target_root_id, key.target_file, key.target);
+        if (!flexible_vars_opt) {
+            (*logger)(fmt::format("Failed to obtain flexible config variables "
+                                  "for absent target {}",
+                                  key.target),
+                      /*fatal=*/true);
             return;
         }
-        (*logger)(fmt::format("Reading target entry for key {} failed",
-                              target_cache_key->Id().ToString()),
-                  /*fatal=*/true);
+        (*setter)(std::move(flexible_vars_opt.value()));
     };
+#endif
+    return AbsentTargetVariablesMap{target_variables, jobs};
+}
+
+auto BuildMaps::Target::CreateAbsentTargetMap(
+    const gsl::not_null<ResultTargetMap*>& result_map,
+    const gsl::not_null<AbsentTargetVariablesMap*>& absent_variables,
+    gsl::not_null<RepositoryConfig*> const& repo_config,
+    gsl::not_null<Statistics*> const& stats,
+    gsl::not_null<Progress*> const& exports_progress,
+    std::size_t jobs) -> AbsentTargetMap {
+#ifndef BOOTSTRAP_BUILD_TOOL
+    auto target_reader =
+        [result_map, repo_config, stats, exports_progress, absent_variables](
+            auto ts, auto setter, auto logger, auto subcaller, auto key) {
+            // assumptions:
+            // - target with absent targets file requested
+            // - ServeApi correctly configured
+            auto const& repo_name = key.target.ToModule().repository;
+            auto target_root_id =
+                repo_config->TargetRoot(repo_name)->GetAbsentTreeId();
+            if (!target_root_id) {
+                (*logger)(fmt::format("Failed to get the target root id for "
+                                      "repository \"{}\"",
+                                      repo_name),
+                          /*fatal=*/true);
+                return;
+            }
+            auto vars_request = AbsentTargetDescription{
+                .target_root_id = *target_root_id,
+                .target_file = *(repo_config->TargetFileName(repo_name)),
+                .target = key.target.GetNamedTarget().name};
+            absent_variables->ConsumeAfterKeysReady(
+                ts,
+                {vars_request},
+                [key,
+                 setter,
+                 repo_config,
+                 logger,
+                 result_map,
+                 stats,
+                 exports_progress,
+                 subcaller](auto const& values) {
+                    WithFlexibleVariables(key,
+                                          *(values[0]),
+                                          repo_config,
+                                          subcaller,
+                                          setter,
+                                          logger,
+                                          result_map,
+                                          stats,
+                                          exports_progress);
+                },
+                [logger, target = key.target](auto const& msg, auto fatal) {
+                    (*logger)(fmt::format("While requested the flexible "
+                                          "variables of {}:\n{}",
+                                          target.ToString(),
+                                          msg),
+                              fatal);
+                });
+        };
 #else
     auto target_reader = [](auto /*ts*/,
                             auto /*setter*/,
diff --git a/src/buildtool/build_engine/target_map/absent_target_map.hpp b/src/buildtool/build_engine/target_map/absent_target_map.hpp
index eb454ab7..5b1196df 100644
--- a/src/buildtool/build_engine/target_map/absent_target_map.hpp
+++ b/src/buildtool/build_engine/target_map/absent_target_map.hpp
@@ -14,6 +14,10 @@
 
 #ifndef INCLUDED_SRC_BUILDTOOL_BUILD_ENGINE_TARGET_MAP_ABSENT_TARGET_MAP_HPP
 #define INCLUDED_SRC_BUILDTOOL_BUILD_ENGINE_TARGET_MAP_ABSENT_TARGET_MAP_HPP
+
+#include <string>
+#include <vector>
+
 #include "gsl/gsl"
 #include "src/buildtool/build_engine/analysed_target/analysed_target.hpp"
 #include "src/buildtool/build_engine/target_map/configured_target.hpp"
@@ -22,15 +26,51 @@
 #include "src/buildtool/common/statistics.hpp"
 #include "src/buildtool/multithreading/async_map_consumer.hpp"
 #include "src/buildtool/progress_reporting/progress.hpp"
+#include "src/utils/cpp/hash_combine.hpp"
 
 namespace BuildMaps::Target {
+
+struct AbsentTargetDescription {
+    std::string target_root_id;
+    std::string target_file;
+    std::string target;
+
+    [[nodiscard]] auto operator==(
+        AbsentTargetDescription const& other) const noexcept -> bool {
+        return target_root_id == other.target_root_id &&
+               target_file == other.target_file && target == other.target;
+    }
+};
+
 using AbsentTargetMap = AsyncMapConsumer<ConfiguredTarget, AnalysedTargetPtr>;
+using AbsentTargetVariablesMap =
+    AsyncMapConsumer<AbsentTargetDescription, std::vector<std::string>>;
+
+auto CreateAbsentTargetVariablesMap(std::size_t jobs = 0)
+    -> AbsentTargetVariablesMap;
 
 auto CreateAbsentTargetMap(const gsl::not_null<ResultTargetMap*>&,
+                           const gsl::not_null<AbsentTargetVariablesMap*>&,
                            gsl::not_null<RepositoryConfig*> const& repo_config,
                            gsl::not_null<Statistics*> const& stats,
                            gsl::not_null<Progress*> const& exports_progress,
                            std::size_t jobs = 0) -> AbsentTargetMap;
 
 }  // namespace BuildMaps::Target
-#endif  // INCLUDED_SRC_BUILDTOOL_BUILD_ENGINE_TARGET_MAP_ABSENT_TARGET_MAP_HPP
+
+namespace std {
+template <>
+struct hash<BuildMaps::Target::AbsentTargetDescription> {
+    [[nodiscard]] auto operator()(
+        const BuildMaps::Target::AbsentTargetDescription& t) const noexcept
+        -> std::size_t {
+        size_t seed{};
+        hash_combine<std::string>(&seed, t.target_root_id);
+        hash_combine<std::string>(&seed, t.target_file);
+        hash_combine<std::string>(&seed, t.target);
+        return seed;
+    }
+};
+}  // namespace std
+
+#endif
diff --git a/src/buildtool/main/analyse.cpp b/src/buildtool/main/analyse.cpp
index 6f63e7ab..ab2313e7 100644
--- a/src/buildtool/main/analyse.cpp
+++ b/src/buildtool/main/analyse.cpp
@@ -116,8 +116,15 @@ namespace Target = BuildMaps::Target;
         Base::CreateRuleMap(&rule_file_map, &expr_map, repo_config, jobs);
     auto source_targets =
         Base::CreateSourceTargetMap(&directory_entries, repo_config, jobs);
-    auto absent_target_map = Target::CreateAbsentTargetMap(
-        result_map, repo_config, stats, &exports_progress, jobs);
+    auto absent_target_variables_map =
+        Target::CreateAbsentTargetVariablesMap(jobs);
+    auto absent_target_map =
+        Target::CreateAbsentTargetMap(result_map,
+                                      &absent_target_variables_map,
+                                      repo_config,
+                                      stats,
+                                      &exports_progress,
+                                      jobs);
     auto target_map = Target::CreateTargetMap(&source_targets,
                                               &targets_file_map,
                                               &rule_map,