Ninja: Improve performance with deeply-dependent custom targets

The commit v3.7.0-rc1~339^2 (Ninja: Fix inter-target order-only
dependencies of custom command, 2016-07-20) might cause performance
degradations for larger projects.  Especially when using custom
commands as an input for each compilation rule (e.g. generated headers).

For reference in the following I am referring to
  Source/cmGlobalNinjaGenerator.cxx:
    -> cmGlobalNinjaGenerator::AppendTargetDependsClosure
    -> cmGlobalNinjaGenerator::ComputeTargetDependsClosure

It turned out that the mentioned commit is doing (indirectly) some
redundant work that might impact performance when generating large
projects.

Imagine the dependency tree of custom targets:

    A
     \
      C - D - E
     /
    B

For each target the transitive closure is calculated recursively, but as
the TargetDependsClosures are only cached on the top most level, everything
downstream has to be recalculated. I.e.

    A->C->D->E
    B->C->D->E

This ultimately leads to a lot of redundant calls to AppendTargetOutputs.
The recursive nature of the algorithm itself is not significant to the
problem, but reducing the work to actually to be done work, eliminates the
performance problem.

This patch changes the way, intermediate results are cached. Rather than
caching the closure of targets, we cache the closure of outputs. Such that
in the example above at B->C the cache already would kick in.

Caching the outputs has one disadvantage that the patch takes care of.
In case of such a structure

    A       E
     \     / \
      C - D   G
     /     \ /
    B       F

the calling order for A would be

    A->C->D->E->G  (at which time G is seen to the recursion)

then the recursion returns until it reaches

    A->C->D->F     (at which the seen G would prevent to recurse down to G)

But this would poison the cache for F with a wrong value (without G).
Hence we use a local result set to ensure the cache is still consistently
populated.

For a large C++ project with around 25k targets this reduced the CMake
configure / generate time from ~40s to ~29s.

Signed-off-by: Matthias Maennich <[email protected]>

Source/cmGlobalNinjaGenerator.cxx

@@ -1037,35 +1037,51 @@ void cmGlobalNinjaGenerator::AppendTargetDepends(
 void cmGlobalNinjaGenerator::AppendTargetDependsClosure(
   cmGeneratorTarget const* target, cmNinjaDeps& outputs)
 {
-  TargetDependsClosureMap::iterator i =
-    this->TargetDependsClosures.find(target);
-  if (i == this->TargetDependsClosures.end()) {
-    TargetDependsClosureMap::value_type e(
-      target, std::set<cmGeneratorTarget const*>());
-    i = this->TargetDependsClosures.insert(e).first;
-    this->ComputeTargetDependsClosure(target, i->second);
-  }
-  std::set<cmGeneratorTarget const*> const& targets = i->second;
-  cmNinjaDeps outs;
-  for (auto tgt : targets) {
-    this->AppendTargetOutputs(tgt, outs);
-  }
-  std::sort(outs.begin(), outs.end());
+  cmNinjaOuts outs;
+  this->AppendTargetDependsClosure(target, outs, true);
+
   outputs.insert(outputs.end(), outs.begin(), outs.end());
 }
 
-void cmGlobalNinjaGenerator::ComputeTargetDependsClosure(
-  cmGeneratorTarget const* target, std::set<cmGeneratorTarget const*>& depends)
+void cmGlobalNinjaGenerator::AppendTargetDependsClosure(
+  cmGeneratorTarget const* target, cmNinjaOuts& outputs, bool omit_self)
 {
-  cmTargetDependSet const& targetDeps = this->GetTargetDirectDepends(target);
-  for (auto targetDep : targetDeps) {
-    if (targetDep->GetType() == cmStateEnums::INTERFACE_LIBRARY) {
-      continue;
-    }
-    if (depends.insert(targetDep).second) {
-      this->ComputeTargetDependsClosure(targetDep, depends);
+
+  // try to locate the target in the cache
+  auto find = this->TargetDependsClosures.lower_bound(target);
+
+  if (find == this->TargetDependsClosures.end() || find->first != target) {
+    // We now calculate the closure outputs by inspecting the dependent
+    // targets recursively.
+    // For that we have to distinguish between a local result set that is only
+    // relevant for filling the cache entries properly isolated and a global
+    // result set that is relevant for the result of the top level call to
+    // AppendTargetDependsClosure.
+    auto const& targetDeps = this->GetTargetDirectDepends(target);
+    cmNinjaOuts this_outs; // this will be the new cache entry
+
+    for (auto const& dep_target : targetDeps) {
+      if (dep_target->GetType() == cmStateEnums::INTERFACE_LIBRARY) {
+        continue;
+      }
+
+      // Collect the dependent targets for _this_ target
+      this->AppendTargetDependsClosure(dep_target, this_outs, false);
     }
+    find = this->TargetDependsClosures.emplace_hint(find, target,
+                                                    std::move(this_outs));
+  }
+
+  // now fill the outputs of the final result from the newly generated cache
+  // entry
+  outputs.insert(find->second.begin(), find->second.end());
+
+  // finally generate the outputs of the target itself, if applicable
+  cmNinjaDeps outs;
+  if (!omit_self) {
+    this->AppendTargetOutputs(target, outs);
   }
+  outputs.insert(outs.begin(), outs.end());
 }
 
 void cmGlobalNinjaGenerator::AddTargetAlias(const std::string& alias,

Source/cmGlobalNinjaGenerator.h

@@ -320,6 +320,8 @@ public:
     cmNinjaTargetDepends depends = DependOnTargetArtifact);
   void AppendTargetDependsClosure(cmGeneratorTarget const* target,
                                   cmNinjaDeps& outputs);
+  void AppendTargetDependsClosure(cmGeneratorTarget const* target,
+                                  cmNinjaOuts& outputs, bool omit_self);
   void AddDependencyToAll(cmGeneratorTarget* target);
   void AddDependencyToAll(const std::string& input);
 
@@ -448,10 +450,7 @@ private:
   typedef std::map<std::string, cmGeneratorTarget*> TargetAliasMap;
   TargetAliasMap TargetAliases;
 
-  typedef std::map<cmGeneratorTarget const*,
-                   std::set<cmGeneratorTarget const*>>
-    TargetDependsClosureMap;
-  TargetDependsClosureMap TargetDependsClosures;
+  std::map<cmGeneratorTarget const*, cmNinjaOuts> TargetDependsClosures;
 
   std::string NinjaCommand;
   std::string NinjaVersion;

Source/cmNinjaTypes.h

@@ -6,6 +6,7 @@
 #include "cmConfigure.h" // IWYU pragma: keep
 
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -16,6 +17,7 @@ enum cmNinjaTargetDepends
 };
 
 typedef std::vector<std::string> cmNinjaDeps;
+typedef std::set<std::string> cmNinjaOuts;
 typedef std::map<std::string, std::string> cmNinjaVars;
 
 #endif // ! cmNinjaTypes_h