From d58620bcf328a500b964fd9190f712c96d6a136e Mon Sep 17 00:00:00 2001 From: Klaus Aehlig Date: Fri, 22 Mar 2024 17:35:17 +0100 Subject: 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. --- src/buildtool/build_engine/target_map/TARGETS | 1 + .../build_engine/target_map/absent_target_map.cpp | 360 ++++++++++++--------- .../build_engine/target_map/absent_target_map.hpp | 42 ++- src/buildtool/main/analyse.cpp | 11 +- .../build_engine/target_map/target_map.test.cpp | 88 ++++- 5 files changed, 333 insertions(+), 169 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& result_map, +#ifndef BOOTSTRAP_BUILD_TOOL + +namespace { +void WithFlexibleVariables( + const BuildMaps::Target::ConfiguredTarget& key, + std::vector flexible_vars, gsl::not_null 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 result_map, gsl::not_null const& stats, - gsl::not_null 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 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> + 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> - 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{ - .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( - *result, - std::vector{}, - std::vector{}, - std::vector{}, - std::unordered_set{flexible_vars->begin(), - flexible_vars->end()}, - std::set{}, - 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{ + .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( + *result, + std::vector{}, + std::vector{}, + std::vector{}, + std::unordered_set{flexible_vars.begin(), + flexible_vars.end()}, + std::set{}, + 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& result_map, + const gsl::not_null& absent_variables, + gsl::not_null const& repo_config, + gsl::not_null const& stats, + gsl::not_null 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 +#include + #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; +using AbsentTargetVariablesMap = + AsyncMapConsumer>; + +auto CreateAbsentTargetVariablesMap(std::size_t jobs = 0) + -> AbsentTargetVariablesMap; auto CreateAbsentTargetMap(const gsl::not_null&, + const gsl::not_null&, gsl::not_null const& repo_config, gsl::not_null const& stats, gsl::not_null 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 { + [[nodiscard]] auto operator()( + const BuildMaps::Target::AbsentTargetDescription& t) const noexcept + -> std::size_t { + size_t seed{}; + hash_combine(&seed, t.target_root_id); + hash_combine(&seed, t.target_file); + hash_combine(&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, diff --git a/test/buildtool/build_engine/target_map/target_map.test.cpp b/test/buildtool/build_engine/target_map/target_map.test.cpp index 47d68894..b7a3a1e7 100644 --- a/test/buildtool/build_engine/target_map/target_map.test.cpp +++ b/test/buildtool/build_engine/target_map/target_map.test.cpp @@ -86,8 +86,15 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, "simple targets", "[target_map]") { BuildMaps::Target::ResultTargetMap result_map{0}; Statistics stats{}; Progress exports_progress{}; - auto absent_target_map = BuildMaps::Target::CreateAbsentTargetMap( - &result_map, &repo_config, &stats, &exports_progress, 0); + auto absent_target_variables_map = + BuildMaps::Target::CreateAbsentTargetVariablesMap(0); + auto absent_target_map = + BuildMaps::Target::CreateAbsentTargetMap(&result_map, + &absent_target_variables_map, + &repo_config, + &stats, + &exports_progress, + 0); auto target_map = BuildMaps::Target::CreateTargetMap(&source, &targets_file_map, @@ -439,8 +446,15 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, BuildMaps::Target::ResultTargetMap result_map{0}; Statistics stats{}; Progress exports_progress{}; - auto absent_target_map = BuildMaps::Target::CreateAbsentTargetMap( - &result_map, &repo_config, &stats, &exports_progress, 0); + auto absent_target_variables_map = + BuildMaps::Target::CreateAbsentTargetVariablesMap(0); + auto absent_target_map = + BuildMaps::Target::CreateAbsentTargetMap(&result_map, + &absent_target_variables_map, + &repo_config, + &stats, + &exports_progress, + 0); auto target_map = BuildMaps::Target::CreateTargetMap(&source, &targets_file_map, @@ -515,8 +529,15 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, BuildMaps::Target::ResultTargetMap result_map{0}; Statistics stats{}; Progress exports_progress{}; - auto absent_target_map = BuildMaps::Target::CreateAbsentTargetMap( - &result_map, &repo_config, &stats, &exports_progress, 0); + auto absent_target_variables_map = + BuildMaps::Target::CreateAbsentTargetVariablesMap(0); + auto absent_target_map = + BuildMaps::Target::CreateAbsentTargetMap(&result_map, + &absent_target_variables_map, + &repo_config, + &stats, + &exports_progress, + 0); auto target_map = BuildMaps::Target::CreateTargetMap(&source, &targets_file_map, @@ -601,8 +622,15 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, "built-in rules", "[target_map]") { BuildMaps::Target::ResultTargetMap result_map{0}; Statistics stats{}; Progress exports_progress{}; - auto absent_target_map = BuildMaps::Target::CreateAbsentTargetMap( - &result_map, &repo_config, &stats, &exports_progress, 0); + auto absent_target_variables_map = + BuildMaps::Target::CreateAbsentTargetVariablesMap(0); + auto absent_target_map = + BuildMaps::Target::CreateAbsentTargetMap(&result_map, + &absent_target_variables_map, + &repo_config, + &stats, + &exports_progress, + 0); auto target_map = BuildMaps::Target::CreateTargetMap(&source, &targets_file_map, @@ -797,8 +825,15 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, "target reference", "[target_map]") { BuildMaps::Target::ResultTargetMap result_map{0}; Statistics stats{}; Progress exports_progress{}; - auto absent_target_map = BuildMaps::Target::CreateAbsentTargetMap( - &result_map, &repo_config, &stats, &exports_progress, 0); + auto absent_target_variables_map = + BuildMaps::Target::CreateAbsentTargetVariablesMap(0); + auto absent_target_map = + BuildMaps::Target::CreateAbsentTargetMap(&result_map, + &absent_target_variables_map, + &repo_config, + &stats, + &exports_progress, + 0); auto target_map = BuildMaps::Target::CreateTargetMap(&source, &targets_file_map, @@ -926,8 +961,15 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, "trees", "[target_map]") { BuildMaps::Target::ResultTargetMap result_map{0}; Statistics stats{}; Progress exports_progress{}; - auto absent_target_map = BuildMaps::Target::CreateAbsentTargetMap( - &result_map, &repo_config, &stats, &exports_progress, 0); + auto absent_target_variables_map = + BuildMaps::Target::CreateAbsentTargetVariablesMap(0); + auto absent_target_map = + BuildMaps::Target::CreateAbsentTargetMap(&result_map, + &absent_target_variables_map, + &repo_config, + &stats, + &exports_progress, + 0); auto target_map = BuildMaps::Target::CreateTargetMap(&source, &targets_file_map, @@ -1021,8 +1063,15 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, BuildMaps::Target::ResultTargetMap result_map{0}; Statistics stats{}; Progress exports_progress{}; - auto absent_target_map = BuildMaps::Target::CreateAbsentTargetMap( - &result_map, &repo_config, &stats, &exports_progress, 0); + auto absent_target_variables_map = + BuildMaps::Target::CreateAbsentTargetVariablesMap(0); + auto absent_target_map = + BuildMaps::Target::CreateAbsentTargetMap(&result_map, + &absent_target_variables_map, + &repo_config, + &stats, + &exports_progress, + 0); auto target_map = BuildMaps::Target::CreateTargetMap(&source, &targets_file_map, @@ -1173,8 +1222,15 @@ TEST_CASE_METHOD(HermeticLocalTestFixture, "wrong arguments", "[target_map]") { BuildMaps::Target::ResultTargetMap result_map{0}; Statistics stats{}; Progress exports_progress{}; - auto absent_target_map = BuildMaps::Target::CreateAbsentTargetMap( - &result_map, &repo_config, &stats, &exports_progress, 0); + auto absent_target_variables_map = + BuildMaps::Target::CreateAbsentTargetVariablesMap(0); + auto absent_target_map = + BuildMaps::Target::CreateAbsentTargetMap(&result_map, + &absent_target_variables_map, + &repo_config, + &stats, + &exports_progress, + 0); auto target_map = BuildMaps::Target::CreateTargetMap(&source, &targets_file_map, -- cgit v1.2.3