From b1c3ae33eacb55a54a7ce28c5a5d6aa8903fbac2 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Wed, 9 Apr 2014 00:05:36 +0200 Subject: [PATCH 1/2] cmTarget: Short-circuit language computation if context independent. Computing the language involves computing the source files, which is an expensive operation. It requires calling cmMakefile::GetOrCreateSource many times, which involves creating and matching on many cmSourceFileLocation objects. Source files of a target may depend on the head-target and the config as of commit e6971df6 (cmTarget: Make the source files depend on the config., 2014-02-13). The results are cached for each context as of commit c5b26f3b (cmTarget: Cache the cmSourceFiles in GetSourceFiles., 2014-04-05). Each target in the build graph causes language computation of all of its dependents with itself as the head-target. This means that for 'core' libraries on which everything depends, the source files are computed once for every transitive target-level-dependee and the result is not cached because the head-target is different. This was observed in the VTK buildsystem. Short circuit the computation for targets which have a source-list that is independent of the head-target. If the source-list has already been computed and the generator expression evaluation reports that it was context-independent, return the only source-list already cached for the target. Reset the short-circuit logic when sources are added and when the link libraries are re-computed. --- Source/cmTarget.cxx | 36 ++++++++++++++++++++++++++++++------ Source/cmTarget.h | 2 ++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/Source/cmTarget.cxx b/Source/cmTarget.cxx index d28ac3f08..4903f8b45 100644 --- a/Source/cmTarget.cxx +++ b/Source/cmTarget.cxx @@ -229,6 +229,7 @@ cmTarget::cmTarget() this->DebugCompileOptionsDone = false; this->DebugCompileDefinitionsDone = false; this->DebugSourcesDone = false; + this->LinkImplementationLanguageIsContextDependent = true; } //---------------------------------------------------------------------------- @@ -461,6 +462,7 @@ void cmTarget::FinishConfigure() //---------------------------------------------------------------------------- void cmTarget::ClearLinkMaps() { + this->LinkImplementationLanguageIsContextDependent = true; this->Internal->LinkImplMap.clear(); this->Internal->LinkInterfaceMap.clear(); this->Internal->LinkClosureMap.clear(); @@ -552,7 +554,7 @@ bool cmTarget::IsBundleOnApple() const } //---------------------------------------------------------------------------- -static void processSources(cmTarget const* tgt, +static bool processSources(cmTarget const* tgt, const std::vector &entries, std::vector &srcs, std::set &uniqueSrcs, @@ -562,6 +564,8 @@ static void processSources(cmTarget const* tgt, { cmMakefile *mf = tgt->GetMakefile(); + bool contextDependent = false; + for (std::vector::const_iterator it = entries.begin(), end = entries.end(); it != end; ++it) { @@ -576,8 +580,12 @@ static void processSources(cmTarget const* tgt, tgt, dagChecker), entrySources); - if (mf->IsGeneratingBuildSystem() - && !(*it)->ge->GetHadContextSensitiveCondition()) + + if ((*it)->ge->GetHadContextSensitiveCondition()) + { + contextDependent = true; + } + else if (mf->IsGeneratingBuildSystem()) { cacheSources = true; } @@ -598,7 +606,7 @@ static void processSources(cmTarget const* tgt, cm->IssueMessage(cmake::FATAL_ERROR, e, tgt->GetBacktrace()); } - return; + return contextDependent; } } if (cacheSources) @@ -629,6 +637,7 @@ static void processSources(cmTarget const* tgt, + usedSources, (*it)->ge->GetBacktrace()); } } + return contextDependent; } //---------------------------------------------------------------------------- @@ -664,7 +673,7 @@ void cmTarget::GetSourceFiles(std::vector &files, "SOURCES", 0, 0); std::set uniqueSrcs; - processSources(this, + bool contextDependentDirectSources = processSources(this, this->Internal->SourceEntries, files, uniqueSrcs, @@ -716,7 +725,8 @@ void cmTarget::GetSourceFiles(std::vector &files, } } - processSources(this, + std::vector::size_type numFilesBefore = files.size(); + bool contextDependentInterfaceSources = processSources(this, this->Internal->CachedLinkInterfaceSourcesEntries[config], files, uniqueSrcs, @@ -725,6 +735,12 @@ void cmTarget::GetSourceFiles(std::vector &files, config, debugSources); + if (!contextDependentDirectSources + && !(contextDependentInterfaceSources && numFilesBefore < files.size())) + { + this->LinkImplementationLanguageIsContextDependent = false; + } + if (!this->Makefile->IsGeneratingBuildSystem()) { deleteAndClear(this->Internal->CachedLinkInterfaceSourcesEntries); @@ -801,6 +817,12 @@ void cmTarget::GetSourceFiles(std::vector &files, // Lookup any existing link implementation for this configuration. TargetConfigPair key(head, cmSystemTools::UpperCase(config)); + if(!this->LinkImplementationLanguageIsContextDependent) + { + files = this->Internal->SourceFilesMap.begin()->second; + return; + } + cmTargetInternals::SourceFilesMapType::iterator it = this->Internal->SourceFilesMap.find(key); if(it != this->Internal->SourceFilesMap.end()) @@ -854,6 +876,7 @@ void cmTarget::AddSources(std::vector const& srcs) if (!srcFiles.empty()) { this->Internal->SourceFilesMap.clear(); + this->LinkImplementationLanguageIsContextDependent = true; cmListFileBacktrace lfbt; this->Makefile->GetBacktrace(lfbt); cmGeneratorExpression ge(lfbt); @@ -989,6 +1012,7 @@ cmSourceFile* cmTarget::AddSource(const std::string& src) == this->Internal->SourceEntries.end()) { this->Internal->SourceFilesMap.clear(); + this->LinkImplementationLanguageIsContextDependent = true; cmListFileBacktrace lfbt; this->Makefile->GetBacktrace(lfbt); cmGeneratorExpression ge(lfbt); diff --git a/Source/cmTarget.h b/Source/cmTarget.h index da9d0a1b5..868e2601b 100644 --- a/Source/cmTarget.h +++ b/Source/cmTarget.h @@ -784,6 +784,8 @@ private: std::string const& suffix, std::string const& name, const char* version) const; + + mutable bool LinkImplementationLanguageIsContextDependent; }; typedef std::map cmTargets; From 4f1c71fdd25f33bd0cdeb2b705723db02f8fddf4 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Wed, 9 Apr 2014 01:32:14 +0200 Subject: [PATCH 2/2] cmTarget: Add all sources traced from custom commands at once. The AddSource method accepts one file and tries to avoiding adding it to the sources-list of the target if it already exists. This involves creating many cmSourceFileLocation objects for matching on existing files, which is an expensive operation. Avoid the searching algorithm by appending the new sources as one group. Generate-time processing of source files will ensure uniqueness. Add a new AddTracedSources for this purpose. The existing AddSources method must process the input for policy CMP0049, but as these source filenames come from cmSourceFile::GetFullPath(), we can forego that extra processing. --- Source/cmGeneratorTarget.cxx | 7 +++++-- Source/cmTarget.cxx | 27 +++++++++++++++++++++++++++ Source/cmTarget.h | 1 + 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/Source/cmGeneratorTarget.cxx b/Source/cmGeneratorTarget.cxx index 0d25a00cc..ec5ce9e93 100644 --- a/Source/cmGeneratorTarget.cxx +++ b/Source/cmGeneratorTarget.cxx @@ -610,6 +610,7 @@ private: std::set SourcesQueued; typedef std::map NameMapType; NameMapType NameMap; + std::vector NewSources; void QueueSource(cmSourceFile* sf); void FollowName(std::string const& name); @@ -698,6 +699,8 @@ void cmTargetTraceDependencies::Trace() } } this->CurrentEntry = 0; + + this->Target->AddTracedSources(this->NewSources); } //---------------------------------------------------------------------------- @@ -707,8 +710,8 @@ void cmTargetTraceDependencies::QueueSource(cmSourceFile* sf) { this->SourceQueue.push(sf); - // Make sure this file is in the target. - this->Target->AddSource(sf->GetFullPath()); + // Make sure this file is in the target at the end. + this->NewSources.push_back(sf->GetFullPath()); } } diff --git a/Source/cmTarget.cxx b/Source/cmTarget.cxx index 4903f8b45..1f8cddb1a 100644 --- a/Source/cmTarget.cxx +++ b/Source/cmTarget.cxx @@ -849,6 +849,33 @@ void cmTarget::GetSourceFiles(std::vector &files, } } +//---------------------------------------------------------------------------- +void cmTarget::AddTracedSources(std::vector const& srcs) +{ + std::string srcFiles; + const char* sep = ""; + for(std::vector::const_iterator i = srcs.begin(); + i != srcs.end(); ++i) + { + std::string filename = *i; + srcFiles += sep; + srcFiles += filename; + sep = ";"; + } + if (!srcFiles.empty()) + { + this->Internal->SourceFilesMap.clear(); + this->LinkImplementationLanguageIsContextDependent = true; + cmListFileBacktrace lfbt; + this->Makefile->GetBacktrace(lfbt); + cmGeneratorExpression ge(lfbt); + cmsys::auto_ptr cge = ge.Parse(srcFiles); + cge->SetEvaluateForBuildsystem(true); + this->Internal->SourceEntries.push_back( + new cmTargetInternals::TargetPropertyEntry(cge)); + } +} + //---------------------------------------------------------------------------- void cmTarget::AddSources(std::vector const& srcs) { diff --git a/Source/cmTarget.h b/Source/cmTarget.h index 868e2601b..45ae48717 100644 --- a/Source/cmTarget.h +++ b/Source/cmTarget.h @@ -144,6 +144,7 @@ public: * Add sources to the target. */ void AddSources(std::vector const& srcs); + void AddTracedSources(std::vector const& srcs); cmSourceFile* AddSourceCMP0049(const std::string& src); cmSourceFile* AddSource(const std::string& src);