[target] Turn many Target::foo_ fields into std::unique_ptr<Foo> ones.
There is only one Target class in GN that has accreted over time
many fields that are only used in a small number of cases. However,
many of them are quite large and complex.
Due to this, the Target::~Target() destructor appears high in profiling
traces, and heap usage is dominated by Target allocation. This CL tries
to reduce the problem by making most of these target-type specific
fields into std::unique_ptr<Foo> members instead.
The actual values are created on demand, with static constant globals
used for defaults for targets that don't need them.
Measurements with an optimized GN build and the Fuchsia source tree
show a reduction of about 200 MiB (5% of 3.8 GiB) in peak RAM usage,
and a reduction of about half a second in "gn gen" time.
For the Chromium build, there is no noticeable difference on either
metrics.
+ Refactor SwiftValues::OnTargetResolved() slightly to avoid
creating un-needed SwiftValues instances when not needed
(i.e. for most targets).
+ Create Target::GeneratedFile structure type to hold all
fields related to generated_file() targets as well.
Measurements:
FUCHSIA BEFORE
Done. Made 173241 targets from 5370 files in 17633ms
Done. Made 173241 targets from 5370 files in 17687ms
Done. Made 173241 targets from 5370 files in 17691ms
Done. Made 173241 targets from 5370 files in 17730ms
Done. Made 173241 targets from 5370 files in 17761ms *
Done. Made 173241 targets from 5370 files in 18130ms
Done. Made 173241 targets from 5370 files in 18306ms
Done. Made 173241 targets from 5370 files in 19193ms
Done. Made 173241 targets from 5370 files in 19232ms
3787204
3799056 *
3803784
FUCHSIA AFTER
Done. Made 173241 targets from 5370 files in 16473ms
Done. Made 173241 targets from 5370 files in 16529ms
Done. Made 173241 targets from 5370 files in 16717ms
Done. Made 173241 targets from 5370 files in 16859ms
Done. Made 173241 targets from 5370 files in 17014ms *
Done. Made 173241 targets from 5370 files in 17192ms
Done. Made 173241 targets from 5370 files in 17543ms
Done. Made 173241 targets from 5370 files in 17538ms
Done. Made 173241 targets from 5370 files in 17946ms
3600740
3609616 *
3616088
Change-Id: Iedc65a3250cf73b82ae0b5d1480c32db9fd507bb
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/12401
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: David Turner <digit@google.com>
diff --git a/src/gn/config_values_extractors.h b/src/gn/config_values_extractors.h
index 6479f21..45b1f25 100644
--- a/src/gn/config_values_extractors.h
+++ b/src/gn/config_values_extractors.h
@@ -30,7 +30,12 @@
// DoSomething(iter.cur());
class ConfigValuesIterator {
public:
- explicit ConfigValuesIterator(const Target* target) : target_(target) {}
+ explicit ConfigValuesIterator(const Target* target) : target_(target) {
+ // If the target doesn't have its own config_values()
+ if (!target->has_config_values()) {
+ cur_index_ = 0;
+ }
+ }
bool done() const {
return cur_index_ >= static_cast<int>(target_->configs().size());
diff --git a/src/gn/rust_values.cc b/src/gn/rust_values.cc
index 6531291..3043458 100644
--- a/src/gn/rust_values.cc
+++ b/src/gn/rust_values.cc
@@ -22,6 +22,8 @@
if (!target->source_types_used().RustSourceUsed()) {
return CRATE_AUTO;
}
+ if (!target->has_rust_values())
+ return CRATE_AUTO;
CrateType crate_type = target->rust_values().crate_type();
if (crate_type != CRATE_AUTO) {
diff --git a/src/gn/swift_values.cc b/src/gn/swift_values.cc
index a9677a6..a70c72f 100644
--- a/src/gn/swift_values.cc
+++ b/src/gn/swift_values.cc
@@ -14,35 +14,46 @@
SwiftValues::~SwiftValues() = default;
-bool SwiftValues::OnTargetResolved(const Target* target, Err* err) {
- if (!FillModuleOuputFile(target, err))
+// static
+bool SwiftValues::OnTargetResolved(Target* target, Err* err) {
+ if (!FillModuleOutputFile(target, err))
return false;
FillModuleDependencies(target);
return true;
}
-void SwiftValues::FillModuleDependencies(const Target* target) {
+// static
+void SwiftValues::FillModuleDependencies(Target* target) {
for (const auto& pair : target->GetDeps(Target::DEPS_LINKED)) {
+ if (!pair.ptr->has_swift_values())
+ continue;
+
if (pair.ptr->toolchain() == target->toolchain() ||
pair.ptr->toolchain()->propagates_configs()) {
- modules_.Append(pair.ptr->swift_values().public_modules().begin(),
- pair.ptr->swift_values().public_modules().end());
+ target->swift_values().modules_.Append(
+ pair.ptr->swift_values().public_modules().begin(),
+ pair.ptr->swift_values().public_modules().end());
}
}
for (const auto& pair : target->public_deps()) {
+ if (!pair.ptr->has_swift_values())
+ continue;
+
if (pair.ptr->toolchain() == target->toolchain() ||
pair.ptr->toolchain()->propagates_configs())
- public_modules_.Append(pair.ptr->swift_values().public_modules().begin(),
- pair.ptr->swift_values().public_modules().end());
+ target->swift_values().public_modules_.Append(
+ pair.ptr->swift_values().public_modules().begin(),
+ pair.ptr->swift_values().public_modules().end());
}
- if (builds_module())
- public_modules_.push_back(target);
+ if (target->swift_values().builds_module())
+ target->swift_values().public_modules_.push_back(target);
}
-bool SwiftValues::FillModuleOuputFile(const Target* target, Err* err) {
+// static
+bool SwiftValues::FillModuleOutputFile(Target* target, Err* err) {
if (!target->IsBinary() || !target->source_types_used().SwiftSourceUsed())
return true;
@@ -63,8 +74,9 @@
return false;
}
- module_output_file_ = std::move(module_output_file);
- module_output_dir_ = module_output_file_as_source.GetDir();
+ SwiftValues& swift_values = target->swift_values();
+ swift_values.module_output_file_ = std::move(module_output_file);
+ swift_values.module_output_dir_ = module_output_file_as_source.GetDir();
return true;
}
diff --git a/src/gn/swift_values.h b/src/gn/swift_values.h
index c529738..3852366 100644
--- a/src/gn/swift_values.h
+++ b/src/gn/swift_values.h
@@ -24,8 +24,8 @@
SwiftValues(const SwiftValues&) = delete;
SwiftValues& operator=(const SwiftValues&) = delete;
- // Called when the target is resolved.
- bool OnTargetResolved(const Target* target, Err* err);
+ // Called when the target is resolved. This may update target->swift_values().
+ static bool OnTargetResolved(Target* target, Err* err);
// Path of the bridging header.
SourceFile& bridge_header() { return bridge_header_; }
@@ -36,7 +36,7 @@
const std::string module_name() const { return module_name_; }
// Returns whether the target generates a .swiftmodule.
- bool builds_module() const { return module_output_file_ != OutputFile(); }
+ bool builds_module() const { return !module_output_file_.value().empty(); }
// Name of the generated .swiftmodule file. Computed when the target
// is resolved.
@@ -60,10 +60,10 @@
private:
// Fill informations about .swiftmodule generated by this target.
- bool FillModuleOuputFile(const Target* target, Err* err);
+ static bool FillModuleOutputFile(Target* target, Err* err);
// Fill dependencies information on other target generating .swiftmodules.
- void FillModuleDependencies(const Target* target);
+ static void FillModuleDependencies(Target* target);
// Name of the optional bridge header used to import Objective-C classes.
// Filled from the target, may be empty even if the target include .swift
diff --git a/src/gn/target.cc b/src/gn/target.cc
index d9c02d3..62f17cd 100644
--- a/src/gn/target.cc
+++ b/src/gn/target.cc
@@ -285,6 +285,116 @@
Target::~Target() = default;
+// A technical note on accessors defined below: Using a static global
+// constant is much faster at runtime than using a static local one.
+//
+// In other words:
+//
+// static const Foo kEmptyFoo;
+//
+// const Foo& Target::foo() const {
+// return foo_ ? *foo_ : kEmptyFoo;
+// }
+//
+// Is considerably faster than:
+//
+// const Foo& Target::foo() const {
+// if (foo_) {
+// return *foo_;
+// } else {
+// static const Foo kEmptyFoo;
+// return kEmptyFoo;
+// }
+// }
+//
+// Because the latter requires relatively expensive atomic operations
+// in the second branch.
+//
+
+static const BundleData kEmptyBundleData;
+
+const BundleData& Target::bundle_data() const {
+ return bundle_data_ ? *bundle_data_ : kEmptyBundleData;
+}
+
+BundleData& Target::bundle_data() {
+ if (!bundle_data_)
+ bundle_data_ = std::make_unique<BundleData>();
+ return *bundle_data_;
+}
+
+static ConfigValues kEmptyConfigValues;
+
+const ConfigValues& Target::config_values() const {
+ return config_values_ ? *config_values_ : kEmptyConfigValues;
+}
+
+ConfigValues& Target::config_values() {
+ if (!config_values_)
+ config_values_ = std::make_unique<ConfigValues>();
+ return *config_values_;
+}
+
+static const ActionValues kEmptyActionValues;
+
+const ActionValues& Target::action_values() const {
+ return action_values_ ? *action_values_ : kEmptyActionValues;
+}
+
+ActionValues& Target::action_values() {
+ if (!action_values_)
+ action_values_ = std::make_unique<ActionValues>();
+ return *action_values_;
+}
+
+static const RustValues kEmptyRustValues;
+
+const RustValues& Target::rust_values() const {
+ return rust_values_ ? *rust_values_ : kEmptyRustValues;
+}
+
+RustValues& Target::rust_values() {
+ if (!rust_values_)
+ rust_values_ = std::make_unique<RustValues>();
+ return *rust_values_;
+}
+
+static const SwiftValues kEmptySwiftValues;
+
+const SwiftValues& Target::swift_values() const {
+ return swift_values_ ? *swift_values_ : kEmptySwiftValues;
+}
+
+SwiftValues& Target::swift_values() {
+ if (!swift_values_)
+ swift_values_ = std::make_unique<SwiftValues>();
+ return *swift_values_;
+}
+
+static const Metadata kEmptyMetadata;
+
+const Metadata& Target::metadata() const {
+ return metadata_ ? *metadata_ : kEmptyMetadata;
+}
+
+Metadata& Target::metadata() {
+ if (!metadata_)
+ metadata_ = std::make_unique<Metadata>();
+ return *metadata_;
+}
+
+static const Target::GeneratedFile kEmptyGeneratedFile;
+
+const Target::GeneratedFile& Target::generated_file() const {
+ return generated_file_ ? *generated_file_ : kEmptyGeneratedFile;
+}
+
+Target::GeneratedFile& Target::generated_file() {
+ if (!generated_file_)
+ generated_file_ = std::make_unique<Target::GeneratedFile>();
+ return *generated_file_;
+}
+
// static
const char* Target::GetStringForOutputType(OutputType type) {
switch (type) {
@@ -393,7 +503,7 @@
if (!FillOutputFiles(err))
return false;
- if (!swift_values_.OnTargetResolved(this, err))
+ if (!SwiftValues::OnTargetResolved(this, err))
return false;
if (!CheckSourceSetLanguages(err))
@@ -785,15 +895,19 @@
continue;
// Direct dependency on a bundle_data target.
- if (pair.ptr->output_type() == BUNDLE_DATA)
- bundle_data_.AddBundleData(pair.ptr);
+ if (pair.ptr->output_type() == BUNDLE_DATA) {
+ bundle_data().AddBundleData(pair.ptr);
+ }
// Recursive bundle_data informations from all dependencies.
- for (auto* target : pair.ptr->bundle_data().bundle_deps())
- bundle_data_.AddBundleData(target);
+ if (pair.ptr->has_bundle_data()) {
+ for (auto* target : pair.ptr->bundle_data().bundle_deps())
+ bundle_data().AddBundleData(target);
+ }
}
- bundle_data_.OnTargetResolved(this);
+ if (has_bundle_data())
+ bundle_data().OnTargetResolved(this);
}
bool Target::FillOutputFiles(Err* err) {
@@ -891,7 +1005,8 @@
// Count anything generated from bundle_data dependencies.
if (output_type_ == CREATE_BUNDLE) {
- if (!bundle_data_.GetOutputFiles(settings(), this, &computed_outputs_, err))
+ if (!bundle_data().GetOutputFiles(settings(), this, &computed_outputs_,
+ err))
return false;
}
@@ -910,10 +1025,13 @@
}
// Also count anything the target has declared to be an output.
- std::vector<SourceFile> outputs_as_sources;
- action_values_.GetOutputsAsSourceFiles(this, &outputs_as_sources);
- for (const SourceFile& out : outputs_as_sources)
- computed_outputs_.push_back(OutputFile(settings()->build_settings(), out));
+ if (action_values_.get()) {
+ std::vector<SourceFile> outputs_as_sources;
+ action_values_->GetOutputsAsSourceFiles(this, &outputs_as_sources);
+ for (const SourceFile& out : outputs_as_sources)
+ computed_outputs_.push_back(
+ OutputFile(settings()->build_settings(), out));
+ }
return true;
}
@@ -930,8 +1048,10 @@
// places must match.
// Track where the current settings came from for issuing errors.
+ bool has_precompiled_headers =
+ config_values_.get() && config_values_->has_precompiled_headers();
const Label* pch_header_settings_from = NULL;
- if (config_values_.has_precompiled_headers())
+ if (has_precompiled_headers)
pch_header_settings_from = &label();
for (ConfigValuesIterator iter(this); !iter.done(); iter.Next()) {
@@ -943,10 +1063,10 @@
if (!cur.has_precompiled_headers())
continue; // This one has no precompiled header info, skip.
- if (config_values_.has_precompiled_headers()) {
+ if (has_precompiled_headers) {
// Already have a precompiled header values, the settings must match.
- if (config_values_.precompiled_header() != cur.precompiled_header() ||
- config_values_.precompiled_source() != cur.precompiled_source()) {
+ if (config_values_->precompiled_header() != cur.precompiled_header() ||
+ config_values_->precompiled_source() != cur.precompiled_source()) {
*err = Err(
defined_from(), "Precompiled header setting conflict.",
"The target " + label().GetUserVisibleName(false) +
@@ -955,8 +1075,8 @@
"\n"
"From " +
pch_header_settings_from->GetUserVisibleName(false) +
- "\n header: " + config_values_.precompiled_header() +
- "\n source: " + config_values_.precompiled_source().value() +
+ "\n header: " + config_values_->precompiled_header() +
+ "\n source: " + config_values_->precompiled_source().value() +
"\n\n"
"From " +
config->label().GetUserVisibleName(false) +
@@ -967,8 +1087,8 @@
} else {
// Have settings from a config, apply them to ourselves.
pch_header_settings_from = &config->label();
- config_values_.set_precompiled_header(cur.precompiled_header());
- config_values_.set_precompiled_source(cur.precompiled_source());
+ config_values().set_precompiled_header(cur.precompiled_header());
+ config_values().set_precompiled_source(cur.precompiled_source());
}
}
@@ -1103,11 +1223,11 @@
// Origin is null because this isn't declared anywhere, and should never
// trigger any errors.
next_walk_keys.push_back(Value(nullptr, ""));
- } else {
+ } else if (has_metadata()) {
// Otherwise, we walk this target and collect the appropriate data.
- if (!metadata_.WalkStep(settings()->build_settings(), keys_to_extract,
- keys_to_walk, rebase_dir, &next_walk_keys,
- ¤t_result, err))
+ if (!metadata().WalkStep(settings()->build_settings(), keys_to_extract,
+ keys_to_walk, rebase_dir, &next_walk_keys,
+ ¤t_result, err))
return false;
}
diff --git a/src/gn/target.h b/src/gn/target.h
index fec9731..ee5061b 100644
--- a/src/gn/target.h
+++ b/src/gn/target.h
@@ -161,8 +161,9 @@
}
// Metadata. Target takes ownership of the resulting scope.
- const Metadata& metadata() const { return metadata_; }
- Metadata& metadata() { return metadata_; }
+ const Metadata& metadata() const;
+ Metadata& metadata();
+ bool has_metadata() const { return metadata_.get(); }
// Get metadata from this target and its dependencies. This is intended to
// be called after the target is resolved.
@@ -177,18 +178,37 @@
// GeneratedFile-related methods.
bool GenerateFile(Err* err) const;
- const Value& contents() const { return contents_; }
- void set_contents(const Value& value) { contents_ = value; }
- const Value& output_conversion() const { return output_conversion_; }
- void set_output_conversion(const Value& value) { output_conversion_ = value; }
-
// Metadata collection methods for GeneratedFile targets.
- const SourceDir& rebase() const { return rebase_; }
- void set_rebase(const SourceDir& value) { rebase_ = value; }
- const std::vector<std::string>& data_keys() const { return data_keys_; }
- std::vector<std::string>& data_keys() { return data_keys_; }
- const std::vector<std::string>& walk_keys() const { return walk_keys_; }
- std::vector<std::string>& walk_keys() { return walk_keys_; }
+ struct GeneratedFile {
+ Value output_conversion_;
+ Value contents_; // Value::NONE if metadata collection should occur.
+ SourceDir rebase_;
+ std::vector<std::string> data_keys_;
+ std::vector<std::string> walk_keys_;
+ };
+ const GeneratedFile& generated_file() const;
+ GeneratedFile& generated_file();
+ bool has_generated_file() const { return generated_file_.get(); }
+
+ const Value& contents() const { return generated_file().contents_; }
+ void set_contents(const Value& value) { generated_file().contents_ = value; }
+ const Value& output_conversion() const {
+ return generated_file().output_conversion_;
+ }
+ void set_output_conversion(const Value& value) {
+ generated_file().output_conversion_ = value;
+ }
+
+ const SourceDir& rebase() const { return generated_file().rebase_; }
+ void set_rebase(const SourceDir& value) { generated_file().rebase_ = value; }
+ const std::vector<std::string>& data_keys() const {
+ return generated_file().data_keys_;
+ }
+ std::vector<std::string>& data_keys() { return generated_file().data_keys_; }
+ const std::vector<std::string>& walk_keys() const {
+ return generated_file().walk_keys_;
+ }
+ std::vector<std::string>& walk_keys() { return generated_file().walk_keys_; }
bool testonly() const { return testonly_; }
void set_testonly(bool value) { testonly_ = value; }
@@ -208,8 +228,9 @@
// Information about the bundle. Only valid for CREATE_BUNDLE target after
// they have been resolved.
- const BundleData& bundle_data() const { return bundle_data_; }
- BundleData& bundle_data() { return bundle_data_; }
+ const BundleData& bundle_data() const;
+ BundleData& bundle_data();
+ bool has_bundle_data() const { return bundle_data_.get(); }
// Returns true if targets depending on this one should have an order
// dependency.
@@ -217,7 +238,7 @@
return output_type_ == ACTION || output_type_ == ACTION_FOREACH ||
output_type_ == COPY_FILES || output_type_ == CREATE_BUNDLE ||
output_type_ == BUNDLE_DATA || output_type_ == GENERATED_FILE ||
- (IsBinary() && swift_values().builds_module());
+ (IsBinary() && has_swift_values() && swift_values().builds_module());
}
// Returns the iterator range which can be used in range-based for loops
@@ -272,17 +293,21 @@
}
// This config represents the configuration set directly on this target.
- ConfigValues& config_values() { return config_values_; }
- const ConfigValues& config_values() const { return config_values_; }
+ ConfigValues& config_values();
+ const ConfigValues& config_values() const;
+ bool has_config_values() const { return config_values_.get(); }
- ActionValues& action_values() { return action_values_; }
- const ActionValues& action_values() const { return action_values_; }
+ ActionValues& action_values();
+ const ActionValues& action_values() const;
+ bool has_action_values() const { return action_values_.get(); }
- SwiftValues& swift_values() { return swift_values_; }
- const SwiftValues& swift_values() const { return swift_values_; }
+ SwiftValues& swift_values();
+ const SwiftValues& swift_values() const;
+ bool has_swift_values() const { return swift_values_.get(); }
- RustValues& rust_values() { return rust_values_; }
- const RustValues& rust_values() const { return rust_values_; }
+ RustValues& rust_values();
+ const RustValues& rust_values() const;
+ bool has_rust_values() const { return rust_values_.get(); }
// Transitive closure of libraries that are depended on by this target
InheritedLibraries& rust_transitive_libs() { return rust_transitive_libs_; }
@@ -444,7 +469,7 @@
bool complete_static_lib_ = false;
bool testonly_ = false;
std::vector<std::string> data_;
- BundleData bundle_data_;
+ std::unique_ptr<BundleData> bundle_data_;
OutputFile write_runtime_deps_output_;
LabelTargetVector private_deps_;
@@ -483,19 +508,19 @@
// Used for all binary targets, and for inputs in regular targets. The
// precompiled header values in this struct will be resolved to the ones to
// use for this target, if precompiled headers are used.
- ConfigValues config_values_;
+ std::unique_ptr<ConfigValues> config_values_;
// Used for action[_foreach] targets.
- ActionValues action_values_;
+ std::unique_ptr<ActionValues> action_values_;
// Used for Rust targets.
- RustValues rust_values_;
+ std::unique_ptr<RustValues> rust_values_;
// Used by all targets, only useful to generate Rust targets though.
InheritedLibraries rust_transitive_libs_;
// User for Swift targets.
- SwiftValues swift_values_;
+ std::unique_ptr<SwiftValues> swift_values_;
// Toolchain used by this target. Null until target is resolved.
const Toolchain* toolchain_ = nullptr;
@@ -506,16 +531,10 @@
OutputFile dependency_output_file_;
std::vector<OutputFile> runtime_outputs_;
- Metadata metadata_;
-
- // GeneratedFile values.
- Value output_conversion_;
- Value contents_; // Value::NONE if metadata collection should occur.
+ std::unique_ptr<Metadata> metadata_;
// GeneratedFile as metadata collection values.
- SourceDir rebase_;
- std::vector<std::string> data_keys_;
- std::vector<std::string> walk_keys_;
+ std::unique_ptr<GeneratedFile> generated_file_;
Target(const Target&) = delete;
Target& operator=(const Target&) = delete;
