[gn] Make ResolvedTargetData stateless Profiling 'gn gen' on Chromium 'out/Default' (36k targets, 64 cores) revealed that ResolvedTargetData::GetTargetInfo was a major bottleneck: - std::__Cr::__shared_mutex_base::lock_shared() consumed 9.01% cycles. - Hash computation (CityHash/hash_memory) consumed 14.35% cycles. Together, about 23% of total CPU cycles were spent on every query acquiring shared_locks and probing shard hash maps, even though targets are queried repeatedly and their pointers are immutable after resolution. As suggested by Matt Stark (msta@), we can simplify this significantly: 1. Move TargetInfo directly into Target as 'std::optional<TargetInfo>'. 2. Initialize 'info_' when Target::OnResolvedWithoutChecks() is called (with a fallback lazy initialization in info() for unit tests that skip OnResolved). 3. Make ResolvedTargetData completely stateless. GetTargetInfo() simply returns '&target->info()' inline with zero locks, zero hashing, and zero dynamic heap allocations for TargetInfo map entries. 4. Because Target owns its TargetInfo directly, the lifetime of TargetInfo perfectly matches Target, eliminating any possibility of use-after-free or address-reuse (ABA) bugs when Targets or ResolvedTargetData instances are destroyed in unit tests. Performance impact on Chromium 'out/Default' (vs origin/main): - Wall-clock time: 3.281s -> 3.173s (-108ms, -3.3%) - User CPU time: 30.87s -> 27.32s (-3.55s, -11.5%) - CPU cycles: 102.91G -> 89.86G (-13.05G cycles, -12.7%) - Instructions: 110.14G -> 102.72G (-7.42G instrs, -6.7%) All 845 unit tests passed. Output ninja files are bit-for-bit identical. TAG=agy CONV=4cb5d931-60a8-4f44-8632-3e8d9fb10a8d Change-Id: Ib23f999522044d5b4b46128491d54ef9061ef4b5 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26480 Reviewed-by: Matt Stark <msta@google.com> Commit-Queue: Takuto Ikuta <tikuta@google.com>
diff --git a/src/gn/resolved_target_data.cc b/src/gn/resolved_target_data.cc index fe906d7..de936c5 100644 --- a/src/gn/resolved_target_data.cc +++ b/src/gn/resolved_target_data.cc
@@ -6,26 +6,6 @@ #include "gn/config_values_extractors.h" -ResolvedTargetData::TargetInfo* ResolvedTargetData::GetTargetInfo( - const Target* target) const { - size_t shard_idx = GetShardIndex(target); - Shard& shard = shards_[shard_idx]; - { - std::shared_lock<std::shared_mutex> lock(shard.mutex); - size_t index = shard.targets.IndexOf(target); - if (index != UniqueVector<const Target*>::kIndexNone) { - return shard.infos[index].get(); - } - } - - std::unique_lock<std::shared_mutex> lock(shard.mutex); - auto ret = shard.targets.PushBackWithIndex(target); - if (ret.first) { - shard.infos.push_back(std::make_unique<TargetInfo>(target)); - } - return shard.infos[ret.second].get(); -} - void ResolvedTargetData::ComputeLibInfo(TargetInfo* info) const { UniqueVector<SourceDir> all_lib_dirs; UniqueVector<LibFile> all_libs;
diff --git a/src/gn/resolved_target_data.h b/src/gn/resolved_target_data.h index e7c6e86..2c0f836 100644 --- a/src/gn/resolved_target_data.h +++ b/src/gn/resolved_target_data.h
@@ -8,7 +8,6 @@ #include <atomic> #include <memory> #include <mutex> -#include <shared_mutex> #include <vector> #include "base/containers/span.h" @@ -167,81 +166,13 @@ } private: - enum class LazyBool : uint8_t { - kUnknown, - kTrue, - kFalse, - }; + using LazyBool = Target::LazyBool; + using TargetInfo = Target::TargetInfo; - // The information associated with a given Target pointer. - struct TargetInfo { - TargetInfo() = default; - - TargetInfo(const Target* target) - : target(target), - deps(target->public_deps(), - target->private_deps(), - target->data_deps()) {} - - const Target* target = nullptr; - ResolvedTargetDeps deps; - mutable std::mutex mutex; - - std::atomic<bool> has_lib_info = false; - std::atomic<bool> has_framework_info = false; - std::atomic<bool> has_hard_deps = false; - std::atomic<bool> has_inherited_libs = false; - std::atomic<bool> has_module_deps_information = false; - std::atomic<bool> has_rust_libs = false; - std::atomic<bool> has_swift_values = false; - std::atomic<bool> has_order_only_deps = false; - std::atomic<LazyBool> does_export_public_inputs = LazyBool::kUnknown; - - // Only valid if |has_lib_info| is true. - std::vector<SourceDir> lib_dirs; - std::vector<LibFile> libs; - - // Only valid if |has_framework_info| is true. - std::vector<SourceDir> framework_dirs; - std::vector<std::string> frameworks; - std::vector<std::string> weak_frameworks; - std::vector<std::string> weak_libraries; - - // Only valid if |has_hard_deps| is true. - TargetSet hard_deps; - - // Only valid if |has_inherited_libs| is true. - std::vector<TargetPublicPair> inherited_libs; - - // Only valid if |has_module_deps_information| is true. - std::vector<TargetPublicPair> module_deps_information; - - // Only valid if |has_rust_libs| is true. - std::vector<TargetPublicPair> rust_inherited_libs; - std::vector<TargetPublicPair> rust_inheritable_libs; - - // Only valid if |has_swift_values| is true. - // Most targets will not have Swift dependencies, so only - // allocate a SwiftValues struct when needed. A null pointer - // indicates empty lists. - struct SwiftValues { - std::vector<const Target*> modules; - std::vector<const Target*> public_modules; - - SwiftValues(std::vector<const Target*> modules, - std::vector<const Target*> public_modules) - : modules(std::move(modules)), - public_modules(std::move(public_modules)) {} - }; - std::unique_ptr<SwiftValues> swift_values; - - // Only valid if |has_order_only_deps| is true. - std::vector<OutputFile> order_only_deps; - }; - - // Retrieve TargetInfo value associated with |target|. Create - // a new empty instance on demand if none is already available. - TargetInfo* GetTargetInfo(const Target* target) const; + // Retrieve TargetInfo value associated with |target|. + TargetInfo* GetTargetInfo(const Target* target) const { + return &target->info(); + } const TargetInfo* GetTargetLibInfo(const Target* target) const { TargetInfo* info = GetTargetInfo(target); @@ -374,30 +305,6 @@ void ComputeRustLibsFor(base::span<const Target*> deps, bool is_public, RustLibsBuilder* rust_libs) const; - - // A { Target* -> TargetInfo } map that will create entries - // on demand (hence the mutable qualifier). Implemented with a - // UniqueVector<> and a parallel vector of unique TargetInfo - // instances for best performance. - // We shard the TargetInfo map to reduce lock contention under the - // high-concurrency parallel writing phase of 'gn gen'. 128 shards is chosen - // as the sweet spot based on benchmarking, providing optimal scaling for - // high-core workstation counts (up to 128 threads) with negligible memory - // overhead from empty shards. - static constexpr size_t kNumShards = 128; - struct Shard { - mutable std::shared_mutex mutex; - UniqueVector<const Target*> targets; - std::vector<std::unique_ptr<TargetInfo>> infos; - }; - - // We use std::hash to distribute targets evenly across shards and avoid - // pointer alignment biases. - size_t GetShardIndex(const Target* target) const { - return std::hash<const Target*>()(target) % kNumShards; - } - - mutable Shard shards_[kNumShards]; }; #endif // TOOLS_GN_RESOLVED_TARGET_DATA_H_
diff --git a/src/gn/target.cc b/src/gn/target.cc index d1eaf09..0e30f41 100644 --- a/src/gn/target.cc +++ b/src/gn/target.cc
@@ -588,6 +588,8 @@ ScopedTrace trace(TraceItem::TRACE_ON_RESOLVED, label()); trace.SetToolchain(settings()->toolchain_label()); + info_.emplace(this); + // Copy this target's own dependent and public configs to the list of configs // applying to it. configs_.Append(all_dependent_configs_.begin(), all_dependent_configs_.end());
diff --git a/src/gn/target.h b/src/gn/target.h index 19972b3..17995f7 100644 --- a/src/gn/target.h +++ b/src/gn/target.h
@@ -5,7 +5,10 @@ #ifndef TOOLS_GN_TARGET_H_ #define TOOLS_GN_TARGET_H_ +#include <atomic> #include <bitset> +#include <mutex> +#include <optional> #include <set> #include <string> #include <utility> @@ -22,14 +25,17 @@ #include "gn/metadata.h" #include "gn/output_file.h" #include "gn/pointer_set.h" +#include "gn/resolved_target_deps.h" #include "gn/rust_values.h" #include "gn/settings.h" #include "gn/source_file.h" #include "gn/swift_values.h" +#include "gn/target_public_pair.h" #include "gn/toolchain.h" #include "gn/unique_vector.h" class DepsIteratorRange; +class ResolvedTargetData; class Settings; class Target; class Toolchain; @@ -622,6 +628,92 @@ // GeneratedFile as metadata collection values. std::unique_ptr<GeneratedFile> generated_file_; + friend class ResolvedTargetData; + + enum class LazyBool : uint8_t { + kUnknown, + kTrue, + kFalse, + }; + + // Cached resolved dependency and link information computed on demand by + // ResolvedTargetData. Owned directly by Target so its lifetime matches + // the target itself without requiring external maps, locks, or generation + // IDs. + struct TargetInfo { + TargetInfo() = default; + + explicit TargetInfo(const Target* target) + : target(target), + deps(target->public_deps(), + target->private_deps(), + target->data_deps()) {} + + const Target* target = nullptr; + ResolvedTargetDeps deps; + mutable std::mutex mutex; + + std::atomic<bool> has_lib_info = false; + std::atomic<bool> has_framework_info = false; + std::atomic<bool> has_hard_deps = false; + std::atomic<bool> has_inherited_libs = false; + std::atomic<bool> has_module_deps_information = false; + std::atomic<bool> has_rust_libs = false; + std::atomic<bool> has_swift_values = false; + std::atomic<bool> has_order_only_deps = false; + std::atomic<LazyBool> does_export_public_inputs = LazyBool::kUnknown; + + // Only valid if |has_lib_info| is true. + std::vector<SourceDir> lib_dirs; + std::vector<LibFile> libs; + + // Only valid if |has_framework_info| is true. + std::vector<SourceDir> framework_dirs; + std::vector<std::string> frameworks; + std::vector<std::string> weak_frameworks; + std::vector<std::string> weak_libraries; + + // Only valid if |has_hard_deps| is true. + TargetSet hard_deps; + + // Only valid if |has_inherited_libs| is true. + std::vector<TargetPublicPair> inherited_libs; + + // Only valid if |has_module_deps_information| is true. + std::vector<TargetPublicPair> module_deps_information; + + // Only valid if |has_rust_libs| is true. + std::vector<TargetPublicPair> rust_inherited_libs; + std::vector<TargetPublicPair> rust_inheritable_libs; + + // Only valid if |has_swift_values| is true. + // Most targets will not have Swift dependencies, so only + // allocate a SwiftValues struct when needed. A null pointer + // indicates empty lists. + struct SwiftValues { + std::vector<const Target*> modules; + std::vector<const Target*> public_modules; + + SwiftValues(std::vector<const Target*> modules, + std::vector<const Target*> public_modules) + : modules(std::move(modules)), + public_modules(std::move(public_modules)) {} + }; + std::unique_ptr<SwiftValues> swift_values; + + // Only valid if |has_order_only_deps| is true. + std::vector<OutputFile> order_only_deps; + }; + + TargetInfo& info() const { + if (!info_.has_value()) { + info_.emplace(this); + } + return *info_; + } + + mutable std::optional<TargetInfo> info_; + Target(const Target&) = delete; Target& operator=(const Target&) = delete; };