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);
