From b1a05d6c762ceb6dbf47126a7ddcedadb45e02f5 Mon Sep 17 00:00:00 2001 From: Florian Jacomme Date: Tue, 1 May 2018 16:17:31 +0200 Subject: Revise implementation of case-insensitive command names Store both the as-written and lower-case command names and use the latter to avoid case-insensitive string comparisons. With this I obtain 2-6% speed increase (on Windows) for the configure step with no significant changes in memory usage. A case-insensitive comparison is a lot slower than just calling `==` because the operator will use things like memcmp, so prefer the latter. The `cmSystemTools::LowerCase` function allocates a new string each time it is called, so before this change we were allocating in: * cmMakefile::Configure two times for each function (to look for `cmake_minimum_required` and `project`) * cmMakefile::ExecuteCommand twice by function by calling cmState::GetCommand and copying the name Now we are only allocating once by function instead of four. --- Source/cmForEachCommand.cxx | 6 +++--- Source/cmFunctionCommand.cxx | 7 +++---- Source/cmIfCommand.cxx | 16 +++++++--------- Source/cmListFileCache.cxx | 8 ++++++++ Source/cmListFileCache.h | 19 +++++++++++++++---- Source/cmMacroCommand.cxx | 6 +++--- Source/cmMakefile.cxx | 23 +++++++++++------------ Source/cmState.cxx | 10 +++++++--- Source/cmState.h | 4 ++++ Source/cmWhileCommand.cxx | 6 +++--- 10 files changed, 64 insertions(+), 41 deletions(-) diff --git a/Source/cmForEachCommand.cxx b/Source/cmForEachCommand.cxx index df288bd..9ff967c 100644 --- a/Source/cmForEachCommand.cxx +++ b/Source/cmForEachCommand.cxx @@ -29,10 +29,10 @@ bool cmForEachFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, cmMakefile& mf, cmExecutionStatus& inStatus) { - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "foreach")) { + if (lff.Name.Lower == "foreach") { // record the number of nested foreach commands this->Depth++; - } else if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endforeach")) { + } else if (lff.Name.Lower == "endforeach") { // if this is the endofreach for this statement if (!this->Depth) { // Remove the function blocker for this scope or bail. @@ -97,7 +97,7 @@ bool cmForEachFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, bool cmForEachFunctionBlocker::ShouldRemove(const cmListFileFunction& lff, cmMakefile& mf) { - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endforeach")) { + if (lff.Name.Lower == "endforeach") { std::vector expandedArguments; mf.ExpandArguments(lff.Arguments, expandedArguments); // if the endforeach has arguments then make sure diff --git a/Source/cmFunctionCommand.cxx b/Source/cmFunctionCommand.cxx index 774df84..67c9e9a 100644 --- a/Source/cmFunctionCommand.cxx +++ b/Source/cmFunctionCommand.cxx @@ -9,7 +9,6 @@ #include "cmMakefile.h" #include "cmPolicies.h" #include "cmState.h" -#include "cmSystemTools.h" // define the class for function commands class cmFunctionHelperCommand : public cmCommand @@ -128,9 +127,9 @@ bool cmFunctionFunctionBlocker::IsFunctionBlocked( { // record commands until we hit the ENDFUNCTION // at the ENDFUNCTION call we shift gears and start looking for invocations - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "function")) { + if (lff.Name.Lower == "function") { this->Depth++; - } else if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endfunction")) { + } else if (lff.Name.Lower == "endfunction") { // if this is the endfunction for this function then execute if (!this->Depth) { // create a new command and add it to cmake @@ -157,7 +156,7 @@ bool cmFunctionFunctionBlocker::IsFunctionBlocked( bool cmFunctionFunctionBlocker::ShouldRemove(const cmListFileFunction& lff, cmMakefile& mf) { - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endfunction")) { + if (lff.Name.Lower == "endfunction") { std::vector expandedArguments; mf.ExpandArguments(lff.Arguments, expandedArguments, this->GetStartingContext().FilePath.c_str()); diff --git a/Source/cmIfCommand.cxx b/Source/cmIfCommand.cxx index 4926f22..ae4041d 100644 --- a/Source/cmIfCommand.cxx +++ b/Source/cmIfCommand.cxx @@ -30,9 +30,9 @@ bool cmIfFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, cmExecutionStatus& inStatus) { // we start by recording all the functions - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "if")) { + if (lff.Name.Lower == "if") { this->ScopeDepth++; - } else if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endif")) { + } else if (lff.Name.Lower == "endif") { this->ScopeDepth--; // if this is the endif for this if statement, then start executing if (!this->ScopeDepth) { @@ -48,15 +48,14 @@ bool cmIfFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, int scopeDepth = 0; for (cmListFileFunction const& func : this->Functions) { // keep track of scope depth - if (!cmSystemTools::Strucmp(func.Name.c_str(), "if")) { + if (func.Name.Lower == "if") { scopeDepth++; } - if (!cmSystemTools::Strucmp(func.Name.c_str(), "endif")) { + if (func.Name.Lower == "endif") { scopeDepth--; } // watch for our state change - if (scopeDepth == 0 && - !cmSystemTools::Strucmp(func.Name.c_str(), "else")) { + if (scopeDepth == 0 && func.Name.Lower == "else") { if (this->ElseSeen) { cmListFileBacktrace bt = mf.GetBacktrace(func); @@ -76,8 +75,7 @@ bool cmIfFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, if (!this->IsBlocking && mf.GetCMakeInstance()->GetTrace()) { mf.PrintCommandTrace(func); } - } else if (scopeDepth == 0 && - !cmSystemTools::Strucmp(func.Name.c_str(), "elseif")) { + } else if (scopeDepth == 0 && func.Name.Lower == "elseif") { if (this->ElseSeen) { cmListFileBacktrace bt = mf.GetBacktrace(func); mf.GetCMakeInstance()->IssueMessage( @@ -163,7 +161,7 @@ bool cmIfFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, bool cmIfFunctionBlocker::ShouldRemove(const cmListFileFunction& lff, cmMakefile&) { - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endif")) { + if (lff.Name.Lower == "endif") { // if the endif has arguments, then make sure // they match the arguments of the matching if if (lff.Arguments.empty() || lff.Arguments == this->Args) { diff --git a/Source/cmListFileCache.cxx b/Source/cmListFileCache.cxx index 64e634f..b468257 100644 --- a/Source/cmListFileCache.cxx +++ b/Source/cmListFileCache.cxx @@ -13,6 +13,14 @@ #include #include +cmCommandContext::cmCommandName& cmCommandContext::cmCommandName::operator=( + std::string const& name) +{ + this->Original = name; + this->Lower = cmSystemTools::LowerCase(name); + return *this; +} + struct cmListFileParser { cmListFileParser(cmListFile* lf, cmListFileBacktrace const& lfbt, diff --git a/Source/cmListFileCache.h b/Source/cmListFileCache.h index 1f9e374..70f7166 100644 --- a/Source/cmListFileCache.h +++ b/Source/cmListFileCache.h @@ -23,11 +23,22 @@ class cmMessenger; struct cmCommandContext { - std::string Name; + struct cmCommandName + { + std::string Lower; + std::string Original; + cmCommandName() {} + cmCommandName(std::string const& name) { *this = name; } + cmCommandName& operator=(std::string const& name); + } Name; long Line; cmCommandContext() - : Name() - , Line(0) + : Line(0) + { + } + cmCommandContext(const char* name, int line) + : Name(name) + , Line(line) { } }; @@ -81,7 +92,7 @@ public: cmListFileContext lfc; lfc.FilePath = fileName; lfc.Line = lfcc.Line; - lfc.Name = lfcc.Name; + lfc.Name = lfcc.Name.Original; return lfc; } }; diff --git a/Source/cmMacroCommand.cxx b/Source/cmMacroCommand.cxx index 6f4b930..23d93a3 100644 --- a/Source/cmMacroCommand.cxx +++ b/Source/cmMacroCommand.cxx @@ -161,9 +161,9 @@ bool cmMacroFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, { // record commands until we hit the ENDMACRO // at the ENDMACRO call we shift gears and start looking for invocations - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "macro")) { + if (lff.Name.Lower == "macro") { this->Depth++; - } else if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endmacro")) { + } else if (lff.Name.Lower == "endmacro") { // if this is the endmacro for this macro then execute if (!this->Depth) { mf.AppendProperty("MACROS", this->Args[0].c_str()); @@ -191,7 +191,7 @@ bool cmMacroFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, bool cmMacroFunctionBlocker::ShouldRemove(const cmListFileFunction& lff, cmMakefile& mf) { - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endmacro")) { + if (lff.Name.Lower == "endmacro") { std::vector expandedArguments; mf.ExpandArguments(lff.Arguments, expandedArguments, this->GetStartingContext().FilePath.c_str()); diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 33e76b2..0d0ba42 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -224,7 +224,7 @@ cmListFileBacktrace cmMakefile::GetBacktrace() const cmListFileBacktrace cmMakefile::GetBacktrace(cmCommandContext const& cc) const { cmListFileContext lfc; - lfc.Name = cc.Name; + lfc.Name = cc.Name.Original; lfc.Line = cc.Line; lfc.FilePath = this->StateSnapshot.GetExecutionListFile(); return this->Backtrace.Push(lfc); @@ -265,7 +265,7 @@ void cmMakefile::PrintCommandTrace(const cmListFileFunction& lff) const std::ostringstream msg; msg << full_path << "(" << lff.Line << "): "; - msg << lff.Name << "("; + msg << lff.Name.Original << "("; bool expand = this->GetCMakeInstance()->GetTraceExpand(); std::string temp; for (cmListFileArgument const& arg : lff.Arguments) { @@ -317,14 +317,13 @@ bool cmMakefile::ExecuteCommand(const cmListFileFunction& lff, return result; } - std::string name = lff.Name; - // Place this call on the call stack. cmMakefileCall stack_manager(this, lff, status); static_cast(stack_manager); // Lookup the command prototype. - if (cmCommand* proto = this->GetState()->GetCommand(name)) { + if (cmCommand* proto = + this->GetState()->GetCommandByExactName(lff.Name.Lower)) { // Clone the prototype. std::unique_ptr pcmd(proto->Clone()); pcmd->SetMakefile(this); @@ -341,7 +340,8 @@ bool cmMakefile::ExecuteCommand(const cmListFileFunction& lff, if (!invokeSucceeded || hadNestedError) { if (!hadNestedError) { // The command invocation requested that we report an error. - std::string const error = name + " " + pcmd->GetError(); + std::string const error = + std::string(lff.Name.Original) + " " + pcmd->GetError(); this->IssueMessage(cmake::FATAL_ERROR, error); } result = false; @@ -356,7 +356,7 @@ bool cmMakefile::ExecuteCommand(const cmListFileFunction& lff, } else { if (!cmSystemTools::GetFatalErrorOccured()) { std::string error = "Unknown CMake command \""; - error += lff.Name; + error += lff.Name.Original; error += "\"."; this->IssueMessage(cmake::FATAL_ERROR, error); result = false; @@ -1454,7 +1454,7 @@ void cmMakefile::Configure() bool hasVersion = false; // search for the right policy command for (cmListFileFunction const& func : listFile.Functions) { - if (cmSystemTools::LowerCase(func.Name) == "cmake_minimum_required") { + if (func.Name.Lower == "cmake_minimum_required") { hasVersion = true; break; } @@ -1481,8 +1481,7 @@ void cmMakefile::Configure() allowedCommands.insert("message"); isProblem = false; for (cmListFileFunction const& func : listFile.Functions) { - std::string name = cmSystemTools::LowerCase(func.Name); - if (allowedCommands.find(name) == allowedCommands.end()) { + if (allowedCommands.find(func.Name.Lower) == allowedCommands.end()) { isProblem = true; break; } @@ -1501,7 +1500,7 @@ void cmMakefile::Configure() bool hasProject = false; // search for a project command for (cmListFileFunction const& func : listFile.Functions) { - if (cmSystemTools::LowerCase(func.Name) == "project") { + if (func.Name.Lower == "project") { hasProject = true; break; } @@ -1509,7 +1508,7 @@ void cmMakefile::Configure() // if no project command is found, add one if (!hasProject) { cmListFileFunction project; - project.Name = "PROJECT"; + project.Name.Lower = "project"; project.Arguments.emplace_back("Project", cmListFileArgument::Unquoted, 0); listFile.Functions.insert(listFile.Functions.begin(), project); diff --git a/Source/cmState.cxx b/Source/cmState.cxx index a93fb11..a57be4d 100644 --- a/Source/cmState.cxx +++ b/Source/cmState.cxx @@ -462,13 +462,17 @@ void cmState::AddScriptedCommand(std::string const& name, cmCommand* command) cmCommand* cmState::GetCommand(std::string const& name) const { - std::string sName = cmSystemTools::LowerCase(name); + return GetCommandByExactName(cmSystemTools::LowerCase(name)); +} + +cmCommand* cmState::GetCommandByExactName(std::string const& name) const +{ std::map::const_iterator pos; - pos = this->ScriptedCommands.find(sName); + pos = this->ScriptedCommands.find(name); if (pos != this->ScriptedCommands.end()) { return pos->second; } - pos = this->BuiltinCommands.find(sName); + pos = this->BuiltinCommands.find(name); if (pos != this->BuiltinCommands.end()) { return pos->second; } diff --git a/Source/cmState.h b/Source/cmState.h index 4c6fc69..38bdfec 100644 --- a/Source/cmState.h +++ b/Source/cmState.h @@ -125,7 +125,11 @@ public: bool GetIsGeneratorMultiConfig() const; void SetIsGeneratorMultiConfig(bool b); + // Returns a command from its name, case insensitive, or nullptr cmCommand* GetCommand(std::string const& name) const; + // Returns a command from its name, or nullptr + cmCommand* GetCommandByExactName(std::string const& name) const; + void AddBuiltinCommand(std::string const& name, cmCommand* command); void AddDisallowedCommand(std::string const& name, cmCommand* command, cmPolicies::PolicyID policy, const char* message); diff --git a/Source/cmWhileCommand.cxx b/Source/cmWhileCommand.cxx index a080034..d5bcfc2 100644 --- a/Source/cmWhileCommand.cxx +++ b/Source/cmWhileCommand.cxx @@ -28,10 +28,10 @@ bool cmWhileFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, cmExecutionStatus& inStatus) { // at end of for each execute recorded commands - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "while")) { + if (lff.Name.Lower == "while") { // record the number of while commands past this one this->Depth++; - } else if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endwhile")) { + } else if (lff.Name.Lower == "endwhile") { // if this is the endwhile for this while loop then execute if (!this->Depth) { // Remove the function blocker for this scope or bail. @@ -117,7 +117,7 @@ bool cmWhileFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, bool cmWhileFunctionBlocker::ShouldRemove(const cmListFileFunction& lff, cmMakefile&) { - if (!cmSystemTools::Strucmp(lff.Name.c_str(), "endwhile")) { + if (lff.Name.Lower == "endwhile") { // if the endwhile has arguments, then make sure // they match the arguments of the matching while if (lff.Arguments.empty() || lff.Arguments == this->Args) { -- cgit v0.12