From 275f2a85b21d781e237dd22252261a0a62d9879b Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Fri, 15 Jan 2016 14:06:45 +0100 Subject: Remove temporary allocations when calling cmGeneratorTarget::GetName. This happens quite often from within comparisons such as in NamedGeneratorTargetFinder or FindGeneratorTargetImpl. It is the top hotspot of both, number of allocations as well as number of temporary allocations - the majority of calls lead to temporary allocations. In raw numbers, this patch removes ~1E6 temporary allocations of 1.5E6 temporary allocations in total when running the cmake daemon on the KDevelop build dir. That is 2/3 of the total. This hotspot was found with heaptrack. --- Source/cmGeneratorTarget.cxx | 2 +- Source/cmGeneratorTarget.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/cmGeneratorTarget.cxx b/Source/cmGeneratorTarget.cxx index b05fb41..ff12320 100644 --- a/Source/cmGeneratorTarget.cxx +++ b/Source/cmGeneratorTarget.cxx @@ -335,7 +335,7 @@ cmState::TargetType cmGeneratorTarget::GetType() const } //---------------------------------------------------------------------------- -std::string cmGeneratorTarget::GetName() const +const std::string& cmGeneratorTarget::GetName() const { return this->Target->GetName(); } diff --git a/Source/cmGeneratorTarget.h b/Source/cmGeneratorTarget.h index bd23477..d96a32c 100644 --- a/Source/cmGeneratorTarget.h +++ b/Source/cmGeneratorTarget.h @@ -55,7 +55,7 @@ public: GetLinkInformation(const std::string& config) const; cmState::TargetType GetType() const; - std::string GetName() const; + const std::string& GetName() const; std::string GetExportName() const; std::vector GetPropertyKeys() const; -- cgit v0.12 From f9599ed42f5bfda35b98936257423f00e260498f Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Fri, 15 Jan 2016 14:17:29 +0100 Subject: Remove temporary allocations by extending the lifetime of the retval. See also Herb Sutter's article on the "most important const": http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/ When running the CMake daemon on the KDevelop build dir, this removes some hundreds of thousands of temporary allocations. This hotspot was found with heaptrack. --- Source/cmGlobalGenerator.cxx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Source/cmGlobalGenerator.cxx b/Source/cmGlobalGenerator.cxx index 2126c71..b9296c1 100644 --- a/Source/cmGlobalGenerator.cxx +++ b/Source/cmGlobalGenerator.cxx @@ -2198,9 +2198,9 @@ cmGlobalGenerator::FindGeneratorTargetImpl(std::string const& name) const { for (unsigned int i = 0; i < this->LocalGenerators.size(); ++i) { - std::vector tgts = + const std::vector& tgts = this->LocalGenerators[i]->GetGeneratorTargets(); - for (std::vector::iterator it = tgts.begin(); + for (std::vector::const_iterator it = tgts.begin(); it != tgts.end(); ++it) { if ((*it)->GetName() == name) @@ -2217,9 +2217,9 @@ cmGlobalGenerator::FindImportedTargetImpl(std::string const& name) const { for (unsigned int i = 0; i < this->Makefiles.size(); ++i) { - std::vector tgts = + const std::vector& tgts = this->Makefiles[i]->GetOwnedImportedTargets(); - for (std::vector::iterator it = tgts.begin(); + for (std::vector::const_iterator it = tgts.begin(); it != tgts.end(); ++it) { if ((*it)->GetName() == name && (*it)->IsImportedGloballyVisible()) @@ -2236,9 +2236,9 @@ cmGeneratorTarget* cmGlobalGenerator::FindImportedGeneratorTargetImpl( { for (unsigned int i = 0; i < this->LocalGenerators.size(); ++i) { - std::vector tgts = + const std::vector& tgts = this->LocalGenerators[i]->GetImportedGeneratorTargets(); - for (std::vector::iterator it = tgts.begin(); + for (std::vector::const_iterator it = tgts.begin(); it != tgts.end(); ++it) { if ((*it)->IsImportedGloballyVisible() && (*it)->GetName() == name) -- cgit v0.12 From ad9394f4fdb69f3f3376d7377fac0a22568aed7c Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Fri, 15 Jan 2016 14:19:33 +0100 Subject: Remove temporary allocations in cmMacroHelper::InvokeInitialPass. This code used to convert std::string's to raw C strings only to put that back into a std::string. This patch thus removes ~70k temporary allocations when running the CMake daemon on KDevelop. This hotspot was found with heaptrack. --- Source/cmMacroCommand.cxx | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/Source/cmMacroCommand.cxx b/Source/cmMacroCommand.cxx index e4026b0..71de7a7 100644 --- a/Source/cmMacroCommand.cxx +++ b/Source/cmMacroCommand.cxx @@ -146,16 +146,14 @@ bool cmMacroHelperCommand::InvokeInitialPass // replace formal arguments for (unsigned int j = 0; j < variables.size(); ++j) { - cmSystemTools::ReplaceString(arg.Value, variables[j].c_str(), - expandedArgs[j].c_str()); + cmSystemTools::ReplaceString(arg.Value, variables[j], + expandedArgs[j]); } // replace argc - cmSystemTools::ReplaceString(arg.Value, "${ARGC}",argcDef.c_str()); + cmSystemTools::ReplaceString(arg.Value, "${ARGC}", argcDef); - cmSystemTools::ReplaceString(arg.Value, "${ARGN}", - expandedArgn.c_str()); - cmSystemTools::ReplaceString(arg.Value, "${ARGV}", - expandedArgv.c_str()); + cmSystemTools::ReplaceString(arg.Value, "${ARGN}", expandedArgn); + cmSystemTools::ReplaceString(arg.Value, "${ARGV}", expandedArgv); // if the current argument of the current function has ${ARGV in it // then try replacing ARGV values @@ -163,8 +161,8 @@ bool cmMacroHelperCommand::InvokeInitialPass { for (unsigned int t = 0; t < expandedArgs.size(); ++t) { - cmSystemTools::ReplaceString(arg.Value, argVs[t].c_str(), - expandedArgs[t].c_str()); + cmSystemTools::ReplaceString(arg.Value, argVs[t], + expandedArgs[t]); } } } -- cgit v0.12 From bd2384f593d0cf2c894ff781c4e5278fff2ac96c Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Fri, 15 Jan 2016 14:37:25 +0100 Subject: Optimize cmMakefile::ExpandVariablesInStringNew. We can remove the temporary allocations required for the default-constructed t_lookup passed into the openstack by refactoring the code slightly. Furthermore, we use a vector instead of a stack, since the latter is based on a deque which is not required for a heap / lifo structure. This patch removes ~215k allocations. This hotspot was found with heaptrack. --- Source/cmMakefile.cxx | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 1b0a99a..ba0d672 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -2832,10 +2832,9 @@ cmake::MessageType cmMakefile::ExpandVariablesInStringNew( const char* last = in; std::string result; result.reserve(source.size()); - std::stack openstack; + std::vector openstack; bool error = false; bool done = false; - openstack.push(t_lookup()); cmake::MessageType mtype = cmake::LOG; cmState* state = this->GetCMakeInstance()->GetState(); @@ -2846,10 +2845,10 @@ cmake::MessageType cmMakefile::ExpandVariablesInStringNew( switch(inc) { case '}': - if(openstack.size() > 1) + if(!openstack.empty()) { - t_lookup var = openstack.top(); - openstack.pop(); + t_lookup var = openstack.back(); + openstack.pop_back(); result.append(last, in - last); std::string const& lookup = result.substr(var.loc); const char* value = NULL; @@ -2970,7 +2969,7 @@ cmake::MessageType cmMakefile::ExpandVariablesInStringNew( last = start; in = start - 1; lookup.loc = result.size(); - openstack.push(lookup); + openstack.push_back(lookup); } break; } @@ -2997,7 +2996,7 @@ cmake::MessageType cmMakefile::ExpandVariablesInStringNew( result.append("\r"); last = next + 1; } - else if(nextc == ';' && openstack.size() == 1) + else if(nextc == ';' && openstack.empty()) { // Handled in ExpandListArgument; pass the backslash literally. } @@ -3065,7 +3064,7 @@ cmake::MessageType cmMakefile::ExpandVariablesInStringNew( /* FALLTHROUGH */ default: { - if(openstack.size() > 1 && + if(!openstack.empty() && !(isalnum(inc) || inc == '_' || inc == '/' || inc == '.' || inc == '+' || inc == '-')) @@ -3074,7 +3073,7 @@ cmake::MessageType cmMakefile::ExpandVariablesInStringNew( errorstr += inc; result.append(last, in - last); errorstr += "\') in a variable name: " - "'" + result.substr(openstack.top().loc) + "'"; + "'" + result.substr(openstack.back().loc) + "'"; mtype = cmake::FATAL_ERROR; error = true; } @@ -3085,7 +3084,7 @@ cmake::MessageType cmMakefile::ExpandVariablesInStringNew( } while(!error && !done && *++in); // Check for open variable references yet. - if(!error && openstack.size() != 1) + if(!error && !openstack.empty()) { // There's an open variable reference waiting. Policy CMP0010 flags // whether this is an error or not. The new parser now enforces -- cgit v0.12 From 70788e92641c4cf4c3f20af607cc6e203203b420 Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Fri, 15 Jan 2016 14:55:22 +0100 Subject: Remove temporary allocations when calling cmHasLiteral{Suf,Pre}fix. When the first argument passed is a std::string, we need to take it by const&, otherwise we copy the string and trigger a temporary allocation. This patch removes a few 10k temporary allocations when running the CMake daemon on the KDevelop build dir. This hotspot was found with heaptrack. --- Source/cmAlgorithms.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/cmAlgorithms.h b/Source/cmAlgorithms.h index ef607d2..54617f3 100644 --- a/Source/cmAlgorithms.h +++ b/Source/cmAlgorithms.h @@ -52,13 +52,13 @@ template size_t cmArraySize(const T (&)[N]) { return N; } template -bool cmHasLiteralPrefix(T str1, const char (&str2)[N]) +bool cmHasLiteralPrefix(const T& str1, const char (&str2)[N]) { return cmHasLiteralPrefixImpl(str1, str2, N - 1); } template -bool cmHasLiteralSuffix(T str1, const char (&str2)[N]) +bool cmHasLiteralSuffix(const T& str1, const char (&str2)[N]) { return cmHasLiteralSuffixImpl(str1, str2, N - 1); } -- cgit v0.12