From e614528ad1e7d053169535cc32fe566f6eb22d41 Mon Sep 17 00:00:00 2001 From: Oleksandr Koval Date: Thu, 1 Oct 2020 14:28:03 +0300 Subject: cmListFileCache: Make cmListFileFunction a shared pointer Passing cmListFileFunction everywhere by-value involves big overhead. Now cmListFileFunction stores std::shared_ptr to the underlying data. --- Source/cmCMakeLanguageCommand.cxx | 12 +++----- Source/cmCPluginAPI.cxx | 9 ++++-- Source/cmForEachCommand.cxx | 2 +- Source/cmFunctionBlocker.cxx | 4 +-- Source/cmFunctionCommand.cxx | 2 +- Source/cmIfCommand.cxx | 23 +++++++-------- Source/cmListFileCache.cxx | 23 ++++++--------- Source/cmListFileCache.h | 57 +++++++++++++++++++++++++++++++++----- Source/cmMacroCommand.cxx | 15 +++++----- Source/cmMakefile.cxx | 42 ++++++++++++++-------------- Source/cmMakefileProfilingData.cxx | 6 ++-- Source/cmMakefileProfilingData.h | 2 +- Source/cmVariableWatchCommand.cxx | 9 +++--- Source/cmWhileCommand.cxx | 2 +- 14 files changed, 123 insertions(+), 85 deletions(-) diff --git a/Source/cmCMakeLanguageCommand.cxx b/Source/cmCMakeLanguageCommand.cxx index 9277c20..789c78d 100644 --- a/Source/cmCMakeLanguageCommand.cxx +++ b/Source/cmCMakeLanguageCommand.cxx @@ -77,18 +77,14 @@ bool cmCMakeLanguageCommandCALL(std::vector const& args, cmMakefile& makefile = status.GetMakefile(); cmListFileContext context = makefile.GetBacktrace().Top(); - cmListFileFunction func; - func.Name = callCommand; - func.Line = context.Line; + std::vector funcArgs; + funcArgs.reserve(args.size() - startArg); // The rest of the arguments are passed to the function call above for (size_t i = startArg; i < args.size(); ++i) { - cmListFileArgument lfarg; - lfarg.Delim = args[i].Delim; - lfarg.Line = context.Line; - lfarg.Value = args[i].Value; - func.Arguments.emplace_back(lfarg); + funcArgs.emplace_back(args[i].Value, args[i].Delim, context.Line); } + cmListFileFunction func{ callCommand, context.Line, std::move(funcArgs) }; if (defer) { if (defer->Id.empty()) { diff --git a/Source/cmCPluginAPI.cxx b/Source/cmCPluginAPI.cxx index ee2960b..8ebf6d2 100644 --- a/Source/cmCPluginAPI.cxx +++ b/Source/cmCPluginAPI.cxx @@ -419,12 +419,15 @@ int CCONV cmExecuteCommand(void* arg, const char* name, int numArgs, const char** args) { cmMakefile* mf = static_cast(arg); - cmListFileFunction lff; - lff.Name = name; + + std::vector lffArgs; + lffArgs.reserve(numArgs); for (int i = 0; i < numArgs; ++i) { // Assume all arguments are quoted. - lff.Arguments.emplace_back(args[i], cmListFileArgument::Quoted, 0); + lffArgs.emplace_back(args[i], cmListFileArgument::Quoted, 0); } + + cmListFileFunction lff{ name, 0, std::move(lffArgs) }; cmExecutionStatus status(*mf); return mf->ExecuteCommand(lff, status); } diff --git a/Source/cmForEachCommand.cxx b/Source/cmForEachCommand.cxx index 42df923..bcacb15 100644 --- a/Source/cmForEachCommand.cxx +++ b/Source/cmForEachCommand.cxx @@ -90,7 +90,7 @@ bool cmForEachFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff, cmMakefile& mf) const { std::vector expandedArguments; - mf.ExpandArguments(lff.Arguments, expandedArguments); + mf.ExpandArguments(lff.Arguments(), expandedArguments); return expandedArguments.empty() || expandedArguments.front() == this->Args.front(); } diff --git a/Source/cmFunctionBlocker.cxx b/Source/cmFunctionBlocker.cxx index 643cd82..d4666d7 100644 --- a/Source/cmFunctionBlocker.cxx +++ b/Source/cmFunctionBlocker.cxx @@ -15,9 +15,9 @@ bool cmFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff, cmExecutionStatus& status) { - if (lff.Name.Lower == this->StartCommandName()) { + if (lff.LowerCaseName() == this->StartCommandName()) { this->ScopeDepth++; - } else if (lff.Name.Lower == this->EndCommandName()) { + } else if (lff.LowerCaseName() == this->EndCommandName()) { this->ScopeDepth--; if (this->ScopeDepth == 0U) { cmMakefile& mf = status.GetMakefile(); diff --git a/Source/cmFunctionCommand.cxx b/Source/cmFunctionCommand.cxx index 46bd057..71c82d6 100644 --- a/Source/cmFunctionCommand.cxx +++ b/Source/cmFunctionCommand.cxx @@ -147,7 +147,7 @@ bool cmFunctionFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff, cmMakefile& mf) const { std::vector expandedArguments; - mf.ExpandArguments(lff.Arguments, expandedArguments); + mf.ExpandArguments(lff.Arguments(), expandedArguments); return expandedArguments.empty() || expandedArguments.front() == this->Args.front(); } diff --git a/Source/cmIfCommand.cxx b/Source/cmIfCommand.cxx index fc257b1..55f6453 100644 --- a/Source/cmIfCommand.cxx +++ b/Source/cmIfCommand.cxx @@ -54,7 +54,7 @@ public: bool cmIfFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff, cmMakefile&) const { - return lff.Arguments.empty() || lff.Arguments == this->Args; + return lff.Arguments().empty() || lff.Arguments() == this->Args; } bool cmIfFunctionBlocker::Replay(std::vector functions, @@ -65,19 +65,19 @@ bool cmIfFunctionBlocker::Replay(std::vector functions, int scopeDepth = 0; for (cmListFileFunction const& func : functions) { // keep track of scope depth - if (func.Name.Lower == "if") { + if (func.LowerCaseName() == "if") { scopeDepth++; } - if (func.Name.Lower == "endif") { + if (func.LowerCaseName() == "endif") { scopeDepth--; } // watch for our state change - if (scopeDepth == 0 && func.Name.Lower == "else") { + if (scopeDepth == 0 && func.LowerCaseName() == "else") { if (this->ElseSeen) { - cmListFileBacktrace elseBT = mf.GetBacktrace().Push( - cmListFileContext{ func.Name.Original, - this->GetStartingContext().FilePath, func.Line }); + cmListFileBacktrace elseBT = mf.GetBacktrace().Push(cmListFileContext{ + func.OriginalName(), this->GetStartingContext().FilePath, + func.Line() }); mf.GetCMakeInstance()->IssueMessage( MessageType::FATAL_ERROR, "A duplicate ELSE command was found inside an IF block.", elseBT); @@ -94,9 +94,10 @@ bool cmIfFunctionBlocker::Replay(std::vector functions, if (!this->IsBlocking && mf.GetCMakeInstance()->GetTrace()) { mf.PrintCommandTrace(func); } - } else if (scopeDepth == 0 && func.Name.Lower == "elseif") { - cmListFileBacktrace elseifBT = mf.GetBacktrace().Push(cmListFileContext{ - func.Name.Original, this->GetStartingContext().FilePath, func.Line }); + } else if (scopeDepth == 0 && func.LowerCaseName() == "elseif") { + cmListFileBacktrace elseifBT = mf.GetBacktrace().Push( + cmListFileContext{ func.OriginalName(), + this->GetStartingContext().FilePath, func.Line() }); if (this->ElseSeen) { mf.GetCMakeInstance()->IssueMessage( MessageType::FATAL_ERROR, @@ -116,7 +117,7 @@ bool cmIfFunctionBlocker::Replay(std::vector functions, std::string errorString; std::vector expandedArguments; - mf.ExpandArguments(func.Arguments, expandedArguments); + mf.ExpandArguments(func.Arguments(), expandedArguments); MessageType messType; diff --git a/Source/cmListFileCache.cxx b/Source/cmListFileCache.cxx index d678b56..70ef5af 100644 --- a/Source/cmListFileCache.cxx +++ b/Source/cmListFileCache.cxx @@ -15,14 +15,6 @@ #include "cmStringAlgorithms.h" #include "cmSystemTools.h" -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 lfbt, @@ -43,7 +35,9 @@ struct cmListFileParser cmMessenger* Messenger; const char* FileName; cmListFileLexer* Lexer; - cmListFileFunction Function; + std::string FunctionName; + long FunctionLine; + std::vector FunctionArguments; enum { SeparationOkay, @@ -141,7 +135,9 @@ bool cmListFileParser::Parse() if (haveNewline) { haveNewline = false; if (this->ParseFunction(token->text, token->line)) { - this->ListFile->Functions.push_back(this->Function); + this->ListFile->Functions.emplace_back( + std::move(this->FunctionName), this->FunctionLine, + std::move(this->FunctionArguments)); } else { return false; } @@ -200,9 +196,8 @@ bool cmListFile::ParseString(const char* str, const char* virtual_filename, bool cmListFileParser::ParseFunction(const char* name, long line) { // Ininitialize a new function call. - this->Function = cmListFileFunction(); - this->Function.Name = name; - this->Function.Line = line; + this->FunctionName = name; + this->FunctionLine = line; // Command name has already been parsed. Read the left paren. cmListFileLexer_Token* token; @@ -297,7 +292,7 @@ bool cmListFileParser::ParseFunction(const char* name, long line) bool cmListFileParser::AddArgument(cmListFileLexer_Token* token, cmListFileArgument::Delimiter delim) { - this->Function.Arguments.emplace_back(token->text, delim, token->line); + this->FunctionArguments.emplace_back(token->text, delim, token->line); if (this->Separation == SeparationOkay) { return true; } diff --git a/Source/cmListFileCache.h b/Source/cmListFileCache.h index 5617536..727fc60 100644 --- a/Source/cmListFileCache.h +++ b/Source/cmListFileCache.h @@ -14,6 +14,7 @@ #include #include "cmStateSnapshot.h" +#include "cmSystemTools.h" /** \class cmListFileCache * \brief A class to cache list file contents. @@ -28,16 +29,19 @@ struct cmCommandContext { struct cmCommandName { - std::string Lower; std::string Original; + std::string Lower; cmCommandName() = default; - cmCommandName(std::string const& name) { *this = name; } - cmCommandName& operator=(std::string const& name); + cmCommandName(std::string name) + : Original(std::move(name)) + , Lower(cmSystemTools::LowerCase(this->Original)) + { + } } Name; long Line = 0; cmCommandContext() = default; - cmCommandContext(const char* name, int line) - : Name(name) + cmCommandContext(std::string name, long line) + : Name(std::move(name)) , Line(line) { } @@ -103,9 +107,48 @@ bool operator<(const cmListFileContext& lhs, const cmListFileContext& rhs); bool operator==(cmListFileContext const& lhs, cmListFileContext const& rhs); bool operator!=(cmListFileContext const& lhs, cmListFileContext const& rhs); -struct cmListFileFunction : public cmCommandContext +class cmListFileFunction { - std::vector Arguments; +public: + cmListFileFunction(std::string name, long line, + std::vector args) + : Impl{ std::make_shared(std::move(name), line, + std::move(args)) } + { + } + + std::string const& OriginalName() const noexcept + { + return this->Impl->Name.Original; + } + + std::string const& LowerCaseName() const noexcept + { + return this->Impl->Name.Lower; + } + + long Line() const noexcept { return this->Impl->Line; } + + std::vector const& Arguments() const noexcept + { + return this->Impl->Arguments; + } + + operator cmCommandContext const&() const noexcept { return *this->Impl; } + +private: + struct Implementation : public cmCommandContext + { + Implementation(std::string name, long line, + std::vector args) + : cmCommandContext{ std::move(name), line } + , Arguments{ std::move(args) } + { + } + std::vector Arguments; + }; + + std::shared_ptr Impl; }; // Represent a backtrace (call stack). Provide value semantics diff --git a/Source/cmMacroCommand.cxx b/Source/cmMacroCommand.cxx index 91a600e..98f88c1 100644 --- a/Source/cmMacroCommand.cxx +++ b/Source/cmMacroCommand.cxx @@ -81,17 +81,14 @@ bool cmMacroHelperCommand::operator()( argVs.emplace_back(argvName); } // Invoke all the functions that were collected in the block. - cmListFileFunction newLFF; // for each function for (cmListFileFunction const& func : this->Functions) { // Replace the formal arguments and then invoke the command. - newLFF.Arguments.clear(); - newLFF.Arguments.reserve(func.Arguments.size()); - newLFF.Name = func.Name; - newLFF.Line = func.Line; + std::vector newLFFArgs; + newLFFArgs.reserve(func.Arguments().size()); // for each argument of the current function - for (cmListFileArgument const& k : func.Arguments) { + for (cmListFileArgument const& k : func.Arguments()) { cmListFileArgument arg; arg.Value = k.Value; if (k.Delim != cmListFileArgument::Bracket) { @@ -116,8 +113,10 @@ bool cmMacroHelperCommand::operator()( } arg.Delim = k.Delim; arg.Line = k.Line; - newLFF.Arguments.push_back(std::move(arg)); + newLFFArgs.push_back(std::move(arg)); } + cmListFileFunction newLFF{ func.OriginalName(), func.Line(), + std::move(newLFFArgs) }; cmExecutionStatus status(makefile); if (!makefile.ExecuteCommand(newLFF, status) || status.GetNestedError()) { // The error message should have already included the call stack @@ -157,7 +156,7 @@ bool cmMacroFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff, cmMakefile& mf) const { std::vector expandedArguments; - mf.ExpandArguments(lff.Arguments, expandedArguments); + mf.ExpandArguments(lff.Arguments(), expandedArguments); return expandedArguments.empty() || expandedArguments[0] == this->Args[0]; } diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index ac3a193..14ec689 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -306,8 +306,8 @@ void cmMakefile::PrintCommandTrace( std::string temp; bool expand = this->GetCMakeInstance()->GetTraceExpand(); - args.reserve(lff.Arguments.size()); - for (cmListFileArgument const& arg : lff.Arguments) { + args.reserve(lff.Arguments().size()); + for (cmListFileArgument const& arg : lff.Arguments()) { if (expand) { temp = arg.Value; this->ExpandVariablesInString(temp); @@ -324,11 +324,11 @@ void cmMakefile::PrintCommandTrace( Json::StreamWriterBuilder builder; builder["indentation"] = ""; val["file"] = full_path; - val["line"] = static_cast(lff.Line); + val["line"] = static_cast(lff.Line()); if (deferId) { val["defer"] = *deferId; } - val["cmd"] = lff.Name.Original; + val["cmd"] = lff.OriginalName(); val["args"] = Json::Value(Json::arrayValue); for (std::string const& arg : args) { val["args"].append(arg); @@ -341,11 +341,11 @@ void cmMakefile::PrintCommandTrace( break; } case cmake::TraceFormat::TRACE_HUMAN: - msg << full_path << "(" << lff.Line << "):"; + msg << full_path << "(" << lff.Line() << "):"; if (deferId) { msg << "DEFERRED:" << *deferId << ":"; } - msg << " " << lff.Name.Original << "("; + msg << " " << lff.OriginalName() << "("; for (std::string const& arg : args) { msg << arg << " "; @@ -451,7 +451,7 @@ bool cmMakefile::ExecuteCommand(const cmListFileFunction& lff, // Lookup the command prototype. if (cmState::Command command = - this->GetState()->GetCommandByExactName(lff.Name.Lower)) { + this->GetState()->GetCommandByExactName(lff.LowerCaseName())) { // Decide whether to invoke the command. if (!cmSystemTools::GetFatalErrorOccured()) { // if trace is enabled, print out invoke information @@ -459,13 +459,13 @@ bool cmMakefile::ExecuteCommand(const cmListFileFunction& lff, this->PrintCommandTrace(lff, this->Backtrace.Top().DeferId); } // Try invoking the command. - bool invokeSucceeded = command(lff.Arguments, status); + bool invokeSucceeded = command(lff.Arguments(), status); bool hadNestedError = status.GetNestedError(); if (!invokeSucceeded || hadNestedError) { if (!hadNestedError) { // The command invocation requested that we report an error. std::string const error = - std::string(lff.Name.Original) + " " + status.GetError(); + std::string(lff.OriginalName()) + " " + status.GetError(); this->IssueMessage(MessageType::FATAL_ERROR, error); } result = false; @@ -477,7 +477,7 @@ bool cmMakefile::ExecuteCommand(const cmListFileFunction& lff, } else { if (!cmSystemTools::GetFatalErrorOccured()) { std::string error = - cmStrCat("Unknown CMake command \"", lff.Name.Original, "\"."); + cmStrCat("Unknown CMake command \"", lff.OriginalName(), "\"."); this->IssueMessage(MessageType::FATAL_ERROR, error); result = false; cmSystemTools::SetFatalErrorOccured(); @@ -1690,7 +1690,7 @@ void cmMakefile::Configure() bool hasVersion = false; // search for the right policy command for (cmListFileFunction const& func : listFile.Functions) { - if (func.Name.Lower == "cmake_minimum_required") { + if (func.LowerCaseName() == "cmake_minimum_required") { hasVersion = true; break; } @@ -1717,7 +1717,7 @@ void cmMakefile::Configure() allowedCommands.insert("message"); isProblem = false; for (cmListFileFunction const& func : listFile.Functions) { - if (!cm::contains(allowedCommands, func.Name.Lower)) { + if (!cm::contains(allowedCommands, func.LowerCaseName())) { isProblem = true; break; } @@ -1737,7 +1737,7 @@ void cmMakefile::Configure() bool hasProject = false; // search for a project command for (cmListFileFunction const& func : listFile.Functions) { - if (func.Name.Lower == "project") { + if (func.LowerCaseName() == "project") { hasProject = true; break; } @@ -1754,12 +1754,12 @@ void cmMakefile::Configure() "CMake is pretending there is a \"project(Project)\" command on " "the first line.", this->Backtrace); - cmListFileFunction project; - project.Name.Lower = "project"; - project.Arguments.emplace_back("Project", cmListFileArgument::Unquoted, - 0); - project.Arguments.emplace_back("__CMAKE_INJECTED_PROJECT_COMMAND__", - cmListFileArgument::Unquoted, 0); + cmListFileFunction project{ "project", + 0, + { { "Project", cmListFileArgument::Unquoted, + 0 }, + { "__CMAKE_INJECTED_PROJECT_COMMAND__", + cmListFileArgument::Unquoted, 0 } } }; listFile.Functions.insert(listFile.Functions.begin(), project); } } @@ -3105,8 +3105,8 @@ cm::optional cmMakefile::DeferGetCall(std::string const& id) const std::string tmp; for (DeferCommand const& dc : this->Defer->Commands) { if (dc.Id == id) { - tmp = dc.Command.Name.Original; - for (cmListFileArgument const& arg : dc.Command.Arguments) { + tmp = dc.Command.OriginalName(); + for (cmListFileArgument const& arg : dc.Command.Arguments()) { tmp = cmStrCat(tmp, ';', arg.Value); } break; diff --git a/Source/cmMakefileProfilingData.cxx b/Source/cmMakefileProfilingData.cxx index 29fd440..86188db 100644 --- a/Source/cmMakefileProfilingData.cxx +++ b/Source/cmMakefileProfilingData.cxx @@ -58,7 +58,7 @@ void cmMakefileProfilingData::StartEntry(const cmListFileFunction& lff, cmsys::SystemInformation info; Json::Value v; v["ph"] = "B"; - v["name"] = lff.Name.Lower; + v["name"] = lff.LowerCaseName(); v["cat"] = "cmake"; v["ts"] = Json::Value::UInt64( std::chrono::duration_cast( @@ -67,9 +67,9 @@ void cmMakefileProfilingData::StartEntry(const cmListFileFunction& lff, v["pid"] = static_cast(info.GetProcessId()); v["tid"] = 0; Json::Value argsValue; - if (!lff.Arguments.empty()) { + if (!lff.Arguments().empty()) { std::string args; - for (const auto& a : lff.Arguments) { + for (auto const& a : lff.Arguments()) { args += (args.empty() ? "" : " ") + a.Value; } argsValue["functionArgs"] = args; diff --git a/Source/cmMakefileProfilingData.h b/Source/cmMakefileProfilingData.h index a3f128b..a86764a 100644 --- a/Source/cmMakefileProfilingData.h +++ b/Source/cmMakefileProfilingData.h @@ -11,7 +11,7 @@ class StreamWriter; } class cmListFileContext; -struct cmListFileFunction; +class cmListFileFunction; class cmMakefileProfilingData { diff --git a/Source/cmVariableWatchCommand.cxx b/Source/cmVariableWatchCommand.cxx index 750b0b5..7c7fbca 100644 --- a/Source/cmVariableWatchCommand.cxx +++ b/Source/cmVariableWatchCommand.cxx @@ -45,20 +45,21 @@ void cmVariableWatchCommandVariableAccessed(const std::string& variable, std::string stack = *mf->GetProperty("LISTFILE_STACK"); if (!data->Command.empty()) { - cmListFileFunction newLFF; cmProp const currentListFile = mf->GetDefinition("CMAKE_CURRENT_LIST_FILE"); const auto fakeLineNo = std::numeric_limits::max(); - newLFF.Arguments = { + + std::vector newLFFArgs{ { variable, cmListFileArgument::Quoted, fakeLineNo }, { accessString, cmListFileArgument::Quoted, fakeLineNo }, { newValue ? newValue : "", cmListFileArgument::Quoted, fakeLineNo }, { *currentListFile, cmListFileArgument::Quoted, fakeLineNo }, { stack, cmListFileArgument::Quoted, fakeLineNo } }; - newLFF.Name = data->Command; - newLFF.Line = fakeLineNo; + + cmListFileFunction newLFF{ data->Command, fakeLineNo, + std::move(newLFFArgs) }; cmExecutionStatus status(*makefile); if (!makefile->ExecuteCommand(newLFF, status)) { cmSystemTools::Error( diff --git a/Source/cmWhileCommand.cxx b/Source/cmWhileCommand.cxx index 2c7a8a7..327c1c7 100644 --- a/Source/cmWhileCommand.cxx +++ b/Source/cmWhileCommand.cxx @@ -54,7 +54,7 @@ cmWhileFunctionBlocker::~cmWhileFunctionBlocker() bool cmWhileFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff, cmMakefile&) const { - return lff.Arguments.empty() || lff.Arguments == this->Args; + return lff.Arguments().empty() || lff.Arguments() == this->Args; } bool cmWhileFunctionBlocker::Replay(std::vector functions, -- cgit v0.12