GN: Don't write stamp files for one rule. When handling input deps, GN would previously always write a stamp file, even if that stamp file just stamped one input file. This is a wasteful extra rule. The previous code was also a bit fragile because it first computed if there was any output via a 6-line conditional, and then wrote that output in a way that the conditions must match the previous computation. This new structure is a little more clear since we accumulate the inputs and then check the size, so nothing must be kept in sync. Savings in Chrome Linux build is a reduction in 388 build rules and 376K of .ninja files. BUG=552701 Review URL: https://codereview.chromium.org/1663813003 Cr-Original-Commit-Position: refs/heads/master@{#373375} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 4445f6179019a0a62cd4a7854723dba7316c5ac0
diff --git a/tools/gn/ninja_action_target_writer_unittest.cc b/tools/gn/ninja_action_target_writer_unittest.cc index 82f42e2..21a7ea5 100644 --- a/tools/gn/ninja_action_target_writer_unittest.cc +++ b/tools/gn/ninja_action_target_writer_unittest.cc
@@ -349,10 +349,9 @@ "${source_file_part} ${rspfile}\n" " description = ACTION //foo:bar()\n" " restat = 1\n" - "build obj/foo/bar.inputdeps.stamp: stamp ../../foo/script.py\n" "\n" "build input1.out: __foo_bar___rule ../../foo/input1.txt" - " | obj/foo/bar.inputdeps.stamp\n" + " | ../../foo/script.py\n" // Necessary for the rspfile defined in the rule. " unique_name = 0\n" // Substitution for the args.
diff --git a/tools/gn/ninja_binary_target_writer_unittest.cc b/tools/gn/ninja_binary_target_writer_unittest.cc index 8aee945..c9b3ebe 100644 --- a/tools/gn/ninja_binary_target_writer_unittest.cc +++ b/tools/gn/ninja_binary_target_writer_unittest.cc
@@ -180,11 +180,10 @@ "target_out_dir = obj/foo\n" "target_output_name = libshlib\n" "\n" - "build obj/foo/shlib.inputdeps.stamp: stamp obj/foo/action.stamp\n" "build obj/foo/libshlib.input1.o: cxx ../../foo/input1.cc" - " || obj/foo/shlib.inputdeps.stamp\n" + " || obj/foo/action.stamp\n" "build obj/foo/libshlib.input2.o: cxx ../../foo/input2.cc" - " || obj/foo/shlib.inputdeps.stamp\n" + " || obj/foo/action.stamp\n" "\n" "build ./libshlib.so.6: solink obj/foo/libshlib.input1.o " // The order-only dependency here is stricly unnecessary since the
diff --git a/tools/gn/ninja_copy_target_writer_unittest.cc b/tools/gn/ninja_copy_target_writer_unittest.cc index e6ec222..dd8c211 100644 --- a/tools/gn/ninja_copy_target_writer_unittest.cc +++ b/tools/gn/ninja_copy_target_writer_unittest.cc
@@ -90,9 +90,7 @@ writer.Run(); const char expected_linux[] = - "build obj/foo/bar.inputdeps.stamp: stamp ../../foo/script.py\n" - "build input1.out: copy ../../foo/input1.txt || " - "obj/foo/bar.inputdeps.stamp\n" + "build input1.out: copy ../../foo/input1.txt || ../../foo/script.py\n" "\n" "build obj/foo/bar.stamp: stamp input1.out\n"; std::string out_str = out.str();
diff --git a/tools/gn/ninja_target_writer.cc b/tools/gn/ninja_target_writer.cc index 6c5d12b..7d9c08a 100644 --- a/tools/gn/ninja_target_writer.cc +++ b/tools/gn/ninja_target_writer.cc
@@ -147,30 +147,79 @@ << "Toolchain not set on target " << target_->label().GetUserVisibleName(true); - // For an action (where we run a script only once) the sources are the same - // as the source prereqs. - bool list_sources_as_input_deps = (target_->output_type() == Target::ACTION); + // ---------- + // Collect all input files that are input deps of this target. Knowing the + // number before writing allows us to either skip writing the input deps + // stamp or optimize it. Use pointers to avoid copies here. + std::vector<const SourceFile*> input_deps_sources; + input_deps_sources.reserve(32); // Actions get implicit dependencies on the script itself. - bool add_script_source_as_dep = - (target_->output_type() == Target::ACTION) || - (target_->output_type() == Target::ACTION_FOREACH); + if (target_->output_type() == Target::ACTION || + target_->output_type() == Target::ACTION_FOREACH) + input_deps_sources.push_back(&target_->action_values().script()); - if (!add_script_source_as_dep && - extra_hard_deps.empty() && - target_->inputs().empty() && - target_->recursive_hard_deps().empty() && - (!list_sources_as_input_deps || target_->sources().empty()) && - target_->toolchain()->deps().empty()) - return OutputFile(); // No input/hard deps. + // Input files. + for (const auto& input : target_->inputs()) + input_deps_sources.push_back(&input); - // One potential optimization is if there are few input dependencies (or - // potentially few sources that depend on these) it's better to just write - // all hard deps on each sources line than have this intermediate stamp. We - // do the stamp file because duplicating all the order-only deps for each - // source file can really explode the ninja file but this won't be the most - // optimal thing in all cases. + // For an action (where we run a script only once) the sources are the same + // as the inputs. For action_foreach, the sources will be operated on + // separately so don't handle them here. + if (target_->output_type() == Target::ACTION) { + for (const auto& source : target_->sources()) + input_deps_sources.push_back(&source); + } + // ---------- + // Collect all target input dependencies of this target as was done for the + // files above. + std::vector<const Target*> input_deps_targets; + input_deps_targets.reserve(32); + + // Hard dependencies that are direct or indirect dependencies. + // These are large (up to 100s), hence why we check other + const std::set<const Target*>& hard_deps(target_->recursive_hard_deps()); + for (const Target* target : hard_deps) + input_deps_targets.push_back(target); + + // Extra hard dependencies passed in. These are usually empty or small, and + // we don't want to duplicate the explicit hard deps of the target. + for (const Target* target : extra_hard_deps) { + if (!hard_deps.count(target)) + input_deps_targets.push_back(target); + } + + // Toolchain dependencies. These must be resolved before doing anything. + // This just writes all toolchain deps for simplicity. If we find that + // toolchains often have more than one dependency, we could consider writing + // a toolchain-specific stamp file and only include the stamp here. + // Note that these are usually empty/small. + const LabelTargetVector& toolchain_deps = target_->toolchain()->deps(); + for (const auto& toolchain_dep : toolchain_deps) { + // This could theoretically duplicate dependencies already in the list, + // but it shouldn't happen in practice, is inconvenient to check for, + // and only results in harmless redundant dependencies listed. + input_deps_targets.push_back(toolchain_dep.ptr); + } + + // --------- + // Write the outputs. + + if (input_deps_sources.size() + input_deps_targets.size() == 0) + return OutputFile(); // No input dependencies. + + // If we're only generating one input dependency, return it directly instead + // of writing a stamp file for it. + if (input_deps_sources.size() == 1 && input_deps_targets.size() == 0) + return OutputFile(settings_->build_settings(), *input_deps_sources[0]); + if (input_deps_sources.size() == 0 && input_deps_targets.size() == 1) { + const OutputFile& dep = input_deps_targets[0]->dependency_output_file(); + DCHECK(!dep.value().empty()); + return dep; + } + + // Make a stamp file. OutputFile input_stamp_file( RebasePath(GetTargetOutputDir(target_).value(), settings_->build_settings()->build_dir(), @@ -184,61 +233,19 @@ << GetNinjaRulePrefixForToolchain(settings_) << Toolchain::ToolTypeToName(Toolchain::TYPE_STAMP); - // Script file (if applicable). - if (add_script_source_as_dep) { + // File input deps. + for (const SourceFile* source : input_deps_sources) { out_ << " "; - path_output_.WriteFile(out_, target_->action_values().script()); + path_output_.WriteFile(out_, *source); } - // Input files are order-only deps. - for (const auto& input : target_->inputs()) { - out_ << " "; - path_output_.WriteFile(out_, input); - } - if (list_sources_as_input_deps) { - for (const auto& source : target_->sources()) { - out_ << " "; - path_output_.WriteFile(out_, source); - } - } - - // The different souces of input deps may duplicate some targets, so uniquify - // them. These are sorted so the generated files are deterministic. - std::vector<const Target*> sorted_deps; - - // Hard dependencies that are direct or indirect dependencies. - // These are large (up to 100s), hence why we check other - const std::set<const Target*>& hard_deps(target_->recursive_hard_deps()); - - // Extra hard dependencies passed in. Note that these are usually empty/small. - for (const Target* target : extra_hard_deps) { - if (!hard_deps.count(target)) - sorted_deps.push_back(target); - } - - // Toolchain dependencies. These must be resolved before doing anything. - // This just writes all toolchain deps for simplicity. If we find that - // toolchains often have more than one dependency, we could consider writing - // a toolchain-specific stamp file and only include the stamp here. - // Note that these are usually empty/small. - const LabelTargetVector& toolchain_deps = target_->toolchain()->deps(); - for (const auto& toolchain_dep : toolchain_deps) { - // This could theoretically duplicate dependencies already in the list, - // but shouldn't happen in practice, is inconvenient to check for, - // and only results in harmless redundant dependencies listed. - if (!hard_deps.count(toolchain_dep.ptr)) - sorted_deps.push_back(toolchain_dep.ptr); - } - - for (const Target* target : hard_deps) { - sorted_deps.push_back(target); - } - + // Target input deps. Sort by label so the output is deterministic (otherwise + // some of the targets will have gone through std::sets which will have + // sorted them by pointer). std::sort( - sorted_deps.begin(), sorted_deps.end(), + input_deps_targets.begin(), input_deps_targets.end(), [](const Target* a, const Target* b) { return a->label() < b->label(); }); - - for (const auto& dep : sorted_deps) { + for (const auto& dep : input_deps_targets) { DCHECK(!dep->dependency_output_file().value().empty()); out_ << " "; path_output_.WriteFile(out_, dep->dependency_output_file());
diff --git a/tools/gn/ninja_target_writer_unittest.cc b/tools/gn/ninja_target_writer_unittest.cc index e46471a..ccb9c7a 100644 --- a/tools/gn/ninja_target_writer_unittest.cc +++ b/tools/gn/ninja_target_writer_unittest.cc
@@ -72,10 +72,10 @@ OutputFile dep = writer.WriteInputDepsStampAndGetDep(std::vector<const Target*>()); - EXPECT_EQ("obj/foo/base.inputdeps.stamp", dep.value()); - EXPECT_EQ("build obj/foo/base.inputdeps.stamp: stamp " - "../../foo/script.py\n", - stream.str()); + // Since there is only one dependency, it should just be returned and + // nothing written to the stream. + EXPECT_EQ("../../foo/script.py", dep.value()); + EXPECT_EQ("", stream.str()); } // Input deps for the target (should depend on the base). @@ -85,6 +85,8 @@ OutputFile dep = writer.WriteInputDepsStampAndGetDep(std::vector<const Target*>()); + // Since there is more than one dependency, a stamp file will be returned + // and the rule for the stamp file will be written to the stream. EXPECT_EQ("obj/foo/target.inputdeps.stamp", dep.value()); EXPECT_EQ("build obj/foo/target.inputdeps.stamp: stamp " "../../foo/input.txt obj/foo/base.stamp\n", @@ -133,8 +135,8 @@ OutputFile dep = writer.WriteInputDepsStampAndGetDep(std::vector<const Target*>()); - EXPECT_EQ("obj/foo/target.inputdeps.stamp", dep.value()); - EXPECT_EQ("build obj/foo/target.inputdeps.stamp: stamp " - "obj/foo/setup.stamp\n", - stream.str()); + // Since there is more than one dependency, a stamp file will be returned + // and the rule for the stamp file will be written to the stream. + EXPECT_EQ("obj/foo/setup.stamp", dep.value()); + EXPECT_EQ("", stream.str()); }