Do not copy parent build_dependency_files_ in Scope constructors. SourceFileSet insert operations appear high in performance profiles, and can be reduced by doing less of them. This is achieved by ensuring that Scope::build_dependency_files_ only contains the source files added by a given scope, instead of the union of the scope itself and of its parents. A new method Scope::CollectBuildDependencyFiles() is introduced to compute the final set of source files for users that need it. Measurements with hyperfine shows that this speeds up `gn gen` x1.05 with a small Fuchsia build plan (7.2s -> 6,8s). Also peak RSS goes from 3.78 GiB to 3.58 GiB (200 MiB saved!). Change-Id: I684b305421ec684e45b02785e70580efa8f1c2ff Reviewed-on: https://gn-review.googlesource.com/c/gn/+/17880 Reviewed-by: Dirk Pranke <dpranke@google.com> Commit-Queue: David Turner <digit@google.com>
diff --git a/src/gn/function_toolchain.cc b/src/gn/function_toolchain.cc index d86e4cc..1945a6a 100644 --- a/src/gn/function_toolchain.cc +++ b/src/gn/function_toolchain.cc
@@ -210,7 +210,7 @@ // This object will actually be copied into the one owned by the toolchain // manager, but that has to be done in the lock. std::unique_ptr<Toolchain> toolchain = std::make_unique<Toolchain>( - scope->settings(), label, scope->build_dependency_files()); + scope->settings(), label, scope->CollectBuildDependencyFiles()); toolchain->set_defined_from(function); toolchain->visibility().SetPublic();
diff --git a/src/gn/functions.cc b/src/gn/functions.cc index adc1ce3..5d958a5 100644 --- a/src/gn/functions.cc +++ b/src/gn/functions.cc
@@ -369,7 +369,7 @@ // Create the new config. std::unique_ptr<Config> config = std::make_unique<Config>( - scope->settings(), label, scope->build_dependency_files()); + scope->settings(), label, scope->CollectBuildDependencyFiles()); config->set_defined_from(function); if (!Visibility::FillItemVisibility(config.get(), scope, err)) return Value(); @@ -909,7 +909,7 @@ // Create the new pool. std::unique_ptr<Pool> pool = std::make_unique<Pool>( - scope->settings(), label, scope->build_dependency_files()); + scope->settings(), label, scope->CollectBuildDependencyFiles()); if (label.name() == "console") { const Settings* settings = scope->settings();
diff --git a/src/gn/scope.cc b/src/gn/scope.cc index 0efe1f9..0e8ec91 100644 --- a/src/gn/scope.cc +++ b/src/gn/scope.cc
@@ -58,22 +58,21 @@ mutable_containing_(parent), settings_(parent->settings()), mode_flags_(0), - item_collector_(nullptr), - build_dependency_files_(parent->build_dependency_files_) {} + item_collector_(nullptr) {} Scope::Scope(const Scope* parent) : const_containing_(parent), mutable_containing_(nullptr), settings_(parent->settings()), mode_flags_(0), - item_collector_(nullptr), - build_dependency_files_(parent->build_dependency_files_) {} + item_collector_(nullptr) {} Scope::~Scope() = default; void Scope::DetachFromContaining() { const_containing_ = nullptr; mutable_containing_ = nullptr; + build_dependency_files_ = CollectBuildDependencyFiles(); } bool Scope::HasValues(SearchNested search_nested) const { @@ -209,6 +208,29 @@ return nullptr; } +SourceFileSet Scope::CollectBuildDependencyFiles() const { + SourceFileSet result; + + // Compute capacity first. + size_t capacity = 0; + const Scope* scope = this; + do { + capacity += scope->build_dependency_files_.size(); + scope = scope->containing(); + } while (scope); + + result.reserve(capacity); + + scope = this; + do { + result.insert(scope->build_dependency_files_.begin(), + scope->build_dependency_files_.end()); + scope = scope->containing(); + } while (scope); + + return result; +} + void Scope::MarkUsed(std::string_view ident) { RecordMap::iterator found = values_.find(ident); if (found == values_.end()) {
diff --git a/src/gn/scope.h b/src/gn/scope.h index b852941..c9db294 100644 --- a/src/gn/scope.h +++ b/src/gn/scope.h
@@ -297,12 +297,12 @@ // set is constructed conservatively, meaning that every file that can // potentially affect this scope is included, but not necessarily every change // to these files will affect this scope. - const SourceFileSet& build_dependency_files() const { - return build_dependency_files_; - } void AddBuildDependencyFile(const SourceFile& build_dependency_file); void AddBuildDependencyFiles(const SourceFileSet& build_dependency_files); + // Collect all dependency files from this scope (and parent ones). + SourceFileSet CollectBuildDependencyFiles() const; + // The item collector is where Items (Targets, Configs, etc.) go that have // been defined. If a scope can generate items, this non-owning pointer will // point to the storage for such items. The creator of this scope will be @@ -413,6 +413,8 @@ SourceDir source_dir_; + // MOTE: build dependency files defined in this scope, not including + // parent ones. SourceFileSet build_dependency_files_; Scope(const Scope&) = delete;
diff --git a/src/gn/scope_unittest.cc b/src/gn/scope_unittest.cc index 9f82685..c16cb3a 100644 --- a/src/gn/scope_unittest.cc +++ b/src/gn/scope_unittest.cc
@@ -26,9 +26,8 @@ bool ContainsBuildDependencyFile(const Scope* scope, const SourceFile& source_file) { - const auto& build_dependency_files = scope->build_dependency_files(); - return build_dependency_files.end() != - build_dependency_files.find(source_file); + const auto& build_dependency_files = scope->CollectBuildDependencyFiles(); + return build_dependency_files.find(source_file) != build_dependency_files.end(); } } // namespace @@ -39,7 +38,7 @@ setup.scope()->AddBuildDependencyFile(source_file); Scope new_scope(setup.scope()); - EXPECT_EQ(1U, new_scope.build_dependency_files().size()); + EXPECT_EQ(1U, new_scope.CollectBuildDependencyFiles().size()); EXPECT_TRUE(ContainsBuildDependencyFile(&new_scope, source_file)); } @@ -225,7 +224,7 @@ EXPECT_TRUE(from_scope.NonRecursiveMergeTo(&to_scope, options, &assignment, "error", &err)); EXPECT_FALSE(err.has_error()); - EXPECT_EQ(1U, to_scope.build_dependency_files().size()); + EXPECT_EQ(1U, to_scope.CollectBuildDependencyFiles().size()); EXPECT_TRUE(ContainsBuildDependencyFile(&to_scope, source_file)); } }
diff --git a/src/gn/setup.cc b/src/gn/setup.cc index c9e6b8b..c2f0aed 100644 --- a/src/gn/setup.cc +++ b/src/gn/setup.cc
@@ -626,7 +626,7 @@ arg_scope.GetCurrentScopeValues(&overrides); build_settings_.build_args().AddArgOverrides(overrides); build_settings_.build_args().set_build_args_dependency_files( - arg_scope.build_dependency_files()); + arg_scope.CollectBuildDependencyFiles()); return true; }
diff --git a/src/gn/target_generator.cc b/src/gn/target_generator.cc index 4138889..59bcf1a 100644 --- a/src/gn/target_generator.cc +++ b/src/gn/target_generator.cc
@@ -94,7 +94,7 @@ g_scheduler->Log("Defining target", label.GetUserVisibleName(true)); std::unique_ptr<Target> target = std::make_unique<Target>( - scope->settings(), label, scope->build_dependency_files()); + scope->settings(), label, scope->CollectBuildDependencyFiles()); target->set_defined_from(function_call); // Create and call out to the proper generator.
diff --git a/src/gn/template.cc b/src/gn/template.cc index 029393c..a01cd8c 100644 --- a/src/gn/template.cc +++ b/src/gn/template.cc
@@ -76,7 +76,7 @@ // Propagate build dependency files from invoker scope (template scope already // propagated via parent scope). template_scope.AddBuildDependencyFiles( - invocation_scope->build_dependency_files()); + invocation_scope->CollectBuildDependencyFiles()); ScopePerFileProvider per_file_provider(&template_scope, true);