From b88f0d2dbd56505ad23d6cfa3bbf6829dedb155f Mon Sep 17 00:00:00 2001 From: elsid Date: Tue, 10 Jan 2023 04:10:18 +0100 Subject: [PATCH] Replace mwscript program serialization into a vector with simple struct Mostly to avoid string literal lookup by index with iteration over all preciding literals and calling strlen. This is very inefficient. In genral this makes code much more straightforward but also makes it portable since now int and float of different sizes are properly supported. --- CHANGELOG.md | 1 + apps/openmw/mwdialogue/dialoguemanagerimp.cpp | 14 +++--- apps/openmw/mwdialogue/dialoguemanagerimp.hpp | 5 +- apps/openmw/mwgui/console.cpp | 5 +- apps/openmw/mwscript/scriptmanagerimp.cpp | 12 ++--- apps/openmw/mwscript/scriptmanagerimp.hpp | 6 +-- .../mwscript/test_scripts.cpp | 8 +--- .../openmw_test_suite/mwscript/test_utils.hpp | 6 +-- components/compiler/fileparser.cpp | 4 +- components/compiler/fileparser.hpp | 3 +- components/compiler/literals.cpp | 48 +------------------ components/compiler/literals.hpp | 13 ++--- components/compiler/output.cpp | 27 +++-------- components/compiler/output.hpp | 4 +- components/compiler/scriptparser.cpp | 4 +- components/compiler/scriptparser.hpp | 3 +- components/interpreter/interpreter.cpp | 17 +++---- components/interpreter/interpreter.hpp | 5 +- components/interpreter/program.hpp | 20 ++++++++ components/interpreter/runtime.cpp | 42 +++++----------- components/interpreter/runtime.hpp | 8 ++-- 21 files changed, 93 insertions(+), 162 deletions(-) create mode 100644 components/interpreter/program.hpp diff --git a/CHANGELOG.md b/CHANGELOG.md index db5cc57e33..352fbae181 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ Feature #7058: Implement TestModels (T3D) console command Feature #7087: Block resolution change in the Windowed Fullscreen mode Feature #7130: Ability to set MyGUI logging verbosity + Feature #7148: Optimize string literal lookup in mwscript 0.48.0 ------ diff --git a/apps/openmw/mwdialogue/dialoguemanagerimp.cpp b/apps/openmw/mwdialogue/dialoguemanagerimp.cpp index 3eb3ded7de..55b4f44fa1 100644 --- a/apps/openmw/mwdialogue/dialoguemanagerimp.cpp +++ b/apps/openmw/mwdialogue/dialoguemanagerimp.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -187,10 +188,10 @@ namespace MWDialogue return false; } - bool DialogueManager::compile( - const std::string& cmd, std::vector& code, const MWWorld::Ptr& actor) + std::optional DialogueManager::compile(const std::string& cmd, const MWWorld::Ptr& actor) { bool success = true; + std::optional program; try { @@ -220,7 +221,7 @@ namespace MWDialogue success = false; if (success) - parser.getCode(code); + program = parser.getProgram(); } catch (const Compiler::SourceException& /* error */) { @@ -238,20 +239,19 @@ namespace MWDialogue Log(Debug::Error) << "Error: compiling failed (dialogue script): \n" << cmd << "\n"; } - return success; + return program; } void DialogueManager::executeScript(const std::string& script, const MWWorld::Ptr& actor) { - std::vector code; - if (compile(script, code, actor)) + if (const std::optional program = compile(script, actor)) { try { MWScript::InterpreterContext interpreterContext(&actor.getRefData().getLocals(), actor); Interpreter::Interpreter interpreter; MWScript::installOpcodes(interpreter); - interpreter.run(code.data(), code.size(), interpreterContext); + interpreter.run(*program, interpreterContext); } catch (const std::exception& error) { diff --git a/apps/openmw/mwdialogue/dialoguemanagerimp.hpp b/apps/openmw/mwdialogue/dialoguemanagerimp.hpp index 0c239acf18..dd5b347c93 100644 --- a/apps/openmw/mwdialogue/dialoguemanagerimp.hpp +++ b/apps/openmw/mwdialogue/dialoguemanagerimp.hpp @@ -4,11 +4,13 @@ #include "../mwbase/dialoguemanager.hpp" #include +#include #include #include #include #include +#include #include #include @@ -63,7 +65,8 @@ namespace MWDialogue void updateActorKnownTopics(); void updateGlobals(); - bool compile(const std::string& cmd, std::vector& code, const MWWorld::Ptr& actor); + std::optional compile(const std::string& cmd, const MWWorld::Ptr& actor); + void executeScript(const std::string& script, const MWWorld::Ptr& actor); void executeTopic(const ESM::RefId& topic, ResponseCallback* callback); diff --git a/apps/openmw/mwgui/console.cpp b/apps/openmw/mwgui/console.cpp index 4e7cdd250b..7bb0065ffa 100644 --- a/apps/openmw/mwgui/console.cpp +++ b/apps/openmw/mwgui/console.cpp @@ -208,9 +208,8 @@ namespace MWGui ConsoleInterpreterContext interpreterContext(*this, mPtr); Interpreter::Interpreter interpreter; MWScript::installOpcodes(interpreter, mConsoleOnlyScripts); - std::vector code; - output.getCode(code); - interpreter.run(code.data(), code.size(), interpreterContext); + const Interpreter::Program program = output.getProgram(); + interpreter.run(program, interpreterContext); } catch (const std::exception& error) { diff --git a/apps/openmw/mwscript/scriptmanagerimp.cpp b/apps/openmw/mwscript/scriptmanagerimp.cpp index 4dfe127411..9668fd344b 100644 --- a/apps/openmw/mwscript/scriptmanagerimp.cpp +++ b/apps/openmw/mwscript/scriptmanagerimp.cpp @@ -79,9 +79,7 @@ namespace MWScript if (Success) { - std::vector code; - mParser.getCode(code); - mScripts.emplace(name, CompiledScript(code, mParser.getLocals())); + mScripts.emplace(name, CompiledScript(mParser.getProgram(), mParser.getLocals())); return true; } @@ -100,8 +98,7 @@ namespace MWScript if (!compile(name)) { // failed -> ignore script from now on. - std::vector empty; - mScripts.emplace(name, CompiledScript(empty, Compiler::Locals())); + mScripts.emplace(name, CompiledScript({}, Compiler::Locals())); return false; } @@ -111,7 +108,8 @@ namespace MWScript // execute script const auto& target = interpreterContext.getTarget(); - if (!iter->second.mByteCode.empty() && iter->second.mInactive.find(target) == iter->second.mInactive.end()) + if (!iter->second.mProgram.mInstructions.empty() + && iter->second.mInactive.find(target) == iter->second.mInactive.end()) try { if (!mOpcodesInstalled) @@ -120,7 +118,7 @@ namespace MWScript mOpcodesInstalled = true; } - mInterpreter.run(&iter->second.mByteCode[0], iter->second.mByteCode.size(), interpreterContext); + mInterpreter.run(iter->second.mProgram, interpreterContext); return true; } catch (const MissingImplicitRefError& e) diff --git a/apps/openmw/mwscript/scriptmanagerimp.hpp b/apps/openmw/mwscript/scriptmanagerimp.hpp index 74b20b222b..de1ce286a6 100644 --- a/apps/openmw/mwscript/scriptmanagerimp.hpp +++ b/apps/openmw/mwscript/scriptmanagerimp.hpp @@ -46,12 +46,12 @@ namespace MWScript struct CompiledScript { - std::vector mByteCode; + Interpreter::Program mProgram; Compiler::Locals mLocals; std::set mInactive; - CompiledScript(const std::vector& code, const Compiler::Locals& locals) - : mByteCode(code) + explicit CompiledScript(Interpreter::Program&& program, const Compiler::Locals& locals) + : mProgram(std::move(program)) , mLocals(locals) { } diff --git a/apps/openmw_test_suite/mwscript/test_scripts.cpp b/apps/openmw_test_suite/mwscript/test_scripts.cpp index 03ad1cd7e8..38ae5e3356 100644 --- a/apps/openmw_test_suite/mwscript/test_scripts.cpp +++ b/apps/openmw_test_suite/mwscript/test_scripts.cpp @@ -21,11 +21,7 @@ namespace Compiler::Scanner scanner(mErrorHandler, input, mCompilerContext.getExtensions()); scanner.scan(mParser); if (mErrorHandler.isGood()) - { - std::vector code; - mParser.getCode(code); - return CompiledScript(code, mParser.getLocals()); - } + return CompiledScript(mParser.getProgram(), mParser.getLocals()); else if (!shouldFail) logErrors(); return {}; @@ -50,7 +46,7 @@ namespace void run(const CompiledScript& script, TestInterpreterContext& context) { - mInterpreter.run(&script.mByteCode[0], static_cast(script.mByteCode.size()), context); + mInterpreter.run(script.mProgram, context); } template diff --git a/apps/openmw_test_suite/mwscript/test_utils.hpp b/apps/openmw_test_suite/mwscript/test_utils.hpp index e5634ffe3d..6f930ba7b3 100644 --- a/apps/openmw_test_suite/mwscript/test_utils.hpp +++ b/apps/openmw_test_suite/mwscript/test_utils.hpp @@ -249,11 +249,11 @@ namespace struct CompiledScript { - std::vector mByteCode; + Interpreter::Program mProgram; Compiler::Locals mLocals; - CompiledScript(const std::vector& code, const Compiler::Locals& locals) - : mByteCode(code) + CompiledScript(Interpreter::Program&& program, const Compiler::Locals& locals) + : mProgram(std::move(program)) , mLocals(locals) { } diff --git a/components/compiler/fileparser.cpp b/components/compiler/fileparser.cpp index 18d245ad9a..d699f54ca4 100644 --- a/components/compiler/fileparser.cpp +++ b/components/compiler/fileparser.cpp @@ -17,9 +17,9 @@ namespace Compiler return mName; } - void FileParser::getCode(std::vector& code) const + Interpreter::Program FileParser::getProgram() const { - mScriptParser.getCode(code); + return mScriptParser.getProgram(); } const Locals& FileParser::getLocals() const diff --git a/components/compiler/fileparser.hpp b/components/compiler/fileparser.hpp index 454f77fec0..19e82344fc 100644 --- a/components/compiler/fileparser.hpp +++ b/components/compiler/fileparser.hpp @@ -31,8 +31,7 @@ namespace Compiler std::string getName() const; ///< Return script name. - void getCode(std::vector& code) const; - ///< store generated code in \a code. + Interpreter::Program getProgram() const; const Locals& getLocals() const; ///< get local variable declarations. diff --git a/components/compiler/literals.cpp b/components/compiler/literals.cpp index 8bcb5bdd57..4aff3cfaf9 100644 --- a/components/compiler/literals.cpp +++ b/components/compiler/literals.cpp @@ -1,56 +1,10 @@ #include "literals.hpp" #include +#include namespace Compiler { - int Literals::getIntegerSize() const - { - return static_cast(mIntegers.size() * sizeof(Interpreter::Type_Integer)); - } - - int Literals::getFloatSize() const - { - return static_cast(mFloats.size() * sizeof(Interpreter::Type_Float)); - } - - int Literals::getStringSize() const - { - int size = 0; - - for (std::vector::const_iterator iter(mStrings.begin()); iter != mStrings.end(); ++iter) - size += static_cast(iter->size()) + 1; - - if (size % 4) // padding - size += 4 - size % 4; - - return size; - } - - void Literals::append(std::vector& code) const - { - for (const int& mInteger : mIntegers) - code.push_back(*reinterpret_cast(&mInteger)); - - for (const float& mFloat : mFloats) - code.push_back(*reinterpret_cast(&mFloat)); - - int stringBlockSize = getStringSize(); - int size = static_cast(code.size()); - - code.resize(size + stringBlockSize / 4); - - size_t offset = 0; - - for (const auto& mString : mStrings) - { - size_t stringSize = mString.size() + 1; - - std::copy(mString.c_str(), mString.c_str() + stringSize, reinterpret_cast(&code[size]) + offset); - offset += stringSize; - } - } - int Literals::addInteger(Interpreter::Type_Integer value) { int index = static_cast(mIntegers.size()); diff --git a/components/compiler/literals.hpp b/components/compiler/literals.hpp index 1dd7761d0d..842af5eda5 100644 --- a/components/compiler/literals.hpp +++ b/components/compiler/literals.hpp @@ -17,18 +17,11 @@ namespace Compiler std::vector mStrings; public: - int getIntegerSize() const; - ///< Return size of integer block (in bytes). + const std::vector& getIntegers() const { return mIntegers; } - int getFloatSize() const; - ///< Return size of float block (in bytes). + const std::vector& getFloats() const { return mFloats; } - int getStringSize() const; - ///< Return size of string block (in bytes). - - void append(std::vector& code) const; - ///< Apepnd literal blocks to code. - /// \note code blocks will be padded for 32-bit alignment. + const std::vector& getStrings() const { return mStrings; } int addInteger(Interpreter::Type_Integer value); ///< add integer liternal and return index. diff --git a/components/compiler/output.cpp b/components/compiler/output.cpp index 877d230258..f19d6a9495 100644 --- a/components/compiler/output.cpp +++ b/components/compiler/output.cpp @@ -13,27 +13,14 @@ namespace Compiler { } - void Output::getCode(std::vector& code) const + Interpreter::Program Output::getProgram() const { - code.clear(); - - // header - code.push_back(static_cast(mCode.size())); - - assert(mLiterals.getIntegerSize() % 4 == 0); - code.push_back(static_cast(mLiterals.getIntegerSize() / 4)); - - assert(mLiterals.getFloatSize() % 4 == 0); - code.push_back(static_cast(mLiterals.getFloatSize() / 4)); - - assert(mLiterals.getStringSize() % 4 == 0); - code.push_back(static_cast(mLiterals.getStringSize() / 4)); - - // code - std::copy(mCode.begin(), mCode.end(), std::back_inserter(code)); - - // literals - mLiterals.append(code); + return Interpreter::Program{ + .mInstructions = mCode, + .mIntegers = mLiterals.getIntegers(), + .mFloats = mLiterals.getFloats(), + .mStrings = mLiterals.getStrings(), + }; } const Literals& Output::getLiterals() const diff --git a/components/compiler/output.hpp b/components/compiler/output.hpp index 1738f386cb..aa36fbf57e 100644 --- a/components/compiler/output.hpp +++ b/components/compiler/output.hpp @@ -5,6 +5,7 @@ #include +#include #include namespace Compiler @@ -20,8 +21,7 @@ namespace Compiler public: Output(Locals& locals); - void getCode(std::vector& code) const; - ///< store generated code in \a code. + Interpreter::Program getProgram() const; const Literals& getLiterals() const; diff --git a/components/compiler/scriptparser.cpp b/components/compiler/scriptparser.cpp index 1b64729a35..d10c56cdd0 100644 --- a/components/compiler/scriptparser.cpp +++ b/components/compiler/scriptparser.cpp @@ -15,9 +15,9 @@ namespace Compiler { } - void ScriptParser::getCode(std::vector& code) const + Interpreter::Program ScriptParser::getProgram() const { - mOutput.getCode(code); + return mOutput.getProgram(); } bool ScriptParser::parseName(const std::string& name, const TokenLoc& loc, Scanner& scanner) diff --git a/components/compiler/scriptparser.hpp b/components/compiler/scriptparser.hpp index 3a7a2738ee..374f5b7264 100644 --- a/components/compiler/scriptparser.hpp +++ b/components/compiler/scriptparser.hpp @@ -23,8 +23,7 @@ namespace Compiler /// \param end of script is marked by end keyword. ScriptParser(ErrorHandler& errorHandler, const Context& context, Locals& locals, bool end = false); - void getCode(std::vector& code) const; - ///< store generated code in \a code. + Interpreter::Program getProgram() const; bool parseName(const std::string& name, const TokenLoc& loc, Scanner& scanner) override; ///< Handle a name token. diff --git a/components/interpreter/interpreter.cpp b/components/interpreter/interpreter.cpp index bf0a9a61c6..6a51a2cddf 100644 --- a/components/interpreter/interpreter.cpp +++ b/components/interpreter/interpreter.cpp @@ -5,6 +5,7 @@ #include #include "opcodes.hpp" +#include "program.hpp" namespace Interpreter { @@ -104,25 +105,19 @@ namespace Interpreter } } - void Interpreter::run(const Type_Code* code, int codeSize, Context& context) + void Interpreter::run(const Program& program, Context& context) { - assert(codeSize >= 4); - begin(); try { - mRuntime.configure(code, codeSize, context); + mRuntime.configure(program, context); - int opcodes = static_cast(code[0]); - - const Type_Code* codeBlock = code + 4; - - while (mRuntime.getPC() >= 0 && mRuntime.getPC() < opcodes) + while (mRuntime.getPC() >= 0 && static_cast(mRuntime.getPC()) < program.mInstructions.size()) { - Type_Code runCode = codeBlock[mRuntime.getPC()]; + const Type_Code instruction = program.mInstructions[mRuntime.getPC()]; mRuntime.setPC(mRuntime.getPC() + 1); - execute(runCode); + execute(instruction); } } catch (...) diff --git a/components/interpreter/interpreter.hpp b/components/interpreter/interpreter.hpp index 911fc716e6..eae3e7b478 100644 --- a/components/interpreter/interpreter.hpp +++ b/components/interpreter/interpreter.hpp @@ -7,12 +7,15 @@ #include #include +#include "components/interpreter/program.hpp" #include "opcodes.hpp" #include "runtime.hpp" #include "types.hpp" namespace Interpreter { + struct Program; + class Interpreter { std::stack mCallstack; @@ -66,7 +69,7 @@ namespace Interpreter installSegment(mSegment5, code, std::make_unique(std::forward(args)...)); } - void run(const Type_Code* code, int codeSize, Context& context); + void run(const Program& program, Context& context); }; } diff --git a/components/interpreter/program.hpp b/components/interpreter/program.hpp new file mode 100644 index 0000000000..58f3cf3c60 --- /dev/null +++ b/components/interpreter/program.hpp @@ -0,0 +1,20 @@ +#ifndef OPENMW_COMPONENTS_INTERPRETER_PROGRAM_H +#define OPENMW_COMPONENTS_INTERPRETER_PROGRAM_H + +#include "types.hpp" + +#include +#include + +namespace Interpreter +{ + struct Program + { + std::vector mInstructions; + std::vector mIntegers; + std::vector mFloats; + std::vector mStrings; + }; +} + +#endif diff --git a/components/interpreter/runtime.cpp b/components/interpreter/runtime.cpp index aa123e443e..20d8aabd92 100644 --- a/components/interpreter/runtime.cpp +++ b/components/interpreter/runtime.cpp @@ -1,4 +1,5 @@ #include "runtime.hpp" +#include "program.hpp" #include #include @@ -8,58 +9,41 @@ namespace Interpreter { int Runtime::getIntegerLiteral(int index) const { - if (index < 0 || index >= static_cast(mCode[1])) - throw std::out_of_range("out of range"); + if (index < 0 || mProgram->mIntegers.size() <= static_cast(index)) + throw std::out_of_range("Invalid integer index"); - const Type_Code* literalBlock = mCode + 4 + mCode[0]; - - return *reinterpret_cast(&literalBlock[index]); + return mProgram->mIntegers[static_cast(index)]; } float Runtime::getFloatLiteral(int index) const { - if (index < 0 || index >= static_cast(mCode[2])) - throw std::out_of_range("out of range"); + if (index < 0 || mProgram->mFloats.size() <= static_cast(index)) + throw std::out_of_range("Invalid float index"); - const Type_Code* literalBlock = mCode + 4 + mCode[0] + mCode[1]; - - return *reinterpret_cast(&literalBlock[index]); + return mProgram->mFloats[static_cast(index)]; } std::string_view Runtime::getStringLiteral(int index) const { - if (index < 0 || static_cast(mCode[3]) <= 0) - throw std::out_of_range("out of range"); + if (index < 0 || mProgram->mStrings.size() <= static_cast(index)) + throw std::out_of_range("Invalid string literal index"); - const char* literalBlock = reinterpret_cast(mCode + 4 + mCode[0] + mCode[1] + mCode[2]); - - size_t offset = 0; - - for (; index; --index) - { - offset += std::strlen(literalBlock + offset) + 1; - if (offset / 4 >= mCode[3]) - throw std::out_of_range("out of range"); - } - - return literalBlock + offset; + return mProgram->mStrings[static_cast(index)]; } - void Runtime::configure(const Type_Code* code, int codeSize, Context& context) + void Runtime::configure(const Program& program, Context& context) { clear(); mContext = &context; - mCode = code; - mCodeSize = codeSize; + mProgram = &program; mPC = 0; } void Runtime::clear() { mContext = nullptr; - mCode = nullptr; - mCodeSize = 0; + mProgram = nullptr; mStack.clear(); } diff --git a/components/interpreter/runtime.hpp b/components/interpreter/runtime.hpp index 33cfe286ae..827fccda11 100644 --- a/components/interpreter/runtime.hpp +++ b/components/interpreter/runtime.hpp @@ -9,14 +9,14 @@ namespace Interpreter { class Context; + struct Program; /// Runtime data and engine interface class Runtime { Context* mContext = nullptr; - const Type_Code* mCode = nullptr; - int mCodeSize = 0; + const Program* mProgram = nullptr; int mPC = 0; std::vector mStack; @@ -30,9 +30,9 @@ namespace Interpreter std::string_view getStringLiteral(int index) const; - void configure(const Type_Code* code, int codeSize, Context& context); + void configure(const Program& program, Context& context); ///< \a context and \a code must exist as least until either configure, clear or - /// the destructor is called. \a codeSize is given in 32-bit words. + /// the destructor is called. void clear();