From d0a82437a4fd18c14544bbf7c787918795a11acc Mon Sep 17 00:00:00 2001 From: Petr Mikheev Date: Sun, 26 Feb 2023 16:50:17 +0100 Subject: [PATCH 1/3] [Lua] Fix memory tracking --- components/lua/luastate.cpp | 5 ++++- components/lua/luastate.hpp | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/components/lua/luastate.cpp b/components/lua/luastate.cpp index d8899a2634..9af617020e 100644 --- a/components/lua/luastate.cpp +++ b/components/lua/luastate.cpp @@ -126,11 +126,14 @@ namespace LuaUtil id = self->mActiveScriptIdStack.back(); bigAllocDelta = nsize; } - if (id.mContainer) + if (id.mIndex >= 0) { if (static_cast(id.mIndex) >= self->mMemoryUsage.size()) self->mMemoryUsage.resize(id.mIndex + 1); self->mMemoryUsage[id.mIndex] += bigAllocDelta; + } + if (id.mContainer) + { id.mContainer->addMemoryUsage(id.mIndex, bigAllocDelta); if (newPtr && nsize > smallAllocSize) self->mBigAllocOwners.emplace(newPtr, AllocOwner{ id.mContainer->mThis, id.mIndex }); diff --git a/components/lua/luastate.hpp b/components/lua/luastate.hpp index b0e173d67e..a41a29d283 100644 --- a/components/lua/luastate.hpp +++ b/components/lua/luastate.hpp @@ -23,7 +23,7 @@ namespace LuaUtil struct ScriptId { ScriptsContainer* mContainer = nullptr; - int mIndex; // index in LuaUtil::ScriptsConfiguration + int mIndex = -1; // index in LuaUtil::ScriptsConfiguration }; struct LuaStateSettings From b0a6e4e510105480d67f0bcc33a942cea3d22aa1 Mon Sep 17 00:00:00 2001 From: Petr Mikheev Date: Sun, 26 Feb 2023 10:55:01 +0100 Subject: [PATCH 2/3] [Lua] Add memory usage test --- apps/openmw_test_suite/lua/test_lua.cpp | 36 ++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/apps/openmw_test_suite/lua/test_lua.cpp b/apps/openmw_test_suite/lua/test_lua.cpp index ebfb76eee1..76d430b440 100644 --- a/apps/openmw_test_suite/lua/test_lua.cpp +++ b/apps/openmw_test_suite/lua/test_lua.cpp @@ -51,10 +51,26 @@ return { } )X"); + std::string genBigScript() + { + std::stringstream buf; + buf << "return function()\n"; + buf << " x = {}\n"; + for (int i = 0; i < 1000; ++i) + buf << " x[" << i * 2 << "] = " << i << "\n"; + buf << " return x\n"; + buf << "end\n"; + return buf.str(); + } + + TestingOpenMW::VFSTestFile bigScriptFile(genBigScript()); + TestingOpenMW::VFSTestFile requireBigScriptFile("local x = require('big') ; return {x}"); + struct LuaStateTest : Test { std::unique_ptr mVFS = TestingOpenMW::createTestVFS({ { "aaa/counter.lua", &counterFile }, - { "bbb/tests.lua", &testsFile }, { "invalid.lua", &invalidScriptFile } }); + { "bbb/tests.lua", &testsFile }, { "invalid.lua", &invalidScriptFile }, { "big.lua", &bigScriptFile }, + { "requireBig.lua", &requireBigScriptFile } }); LuaUtil::ScriptsConfiguration mCfg; LuaUtil::LuaState mLua{ mVFS.get(), &mCfg }; @@ -172,4 +188,22 @@ return { EXPECT_THAT(LuaUtil::getLuaVersion(), HasSubstr("Lua")); } + TEST_F(LuaStateTest, RemovedScriptsGarbageCollecting) + { + auto getMem = [&] { + for (int i = 0; i < 5; ++i) + lua_gc(mLua.sol(), LUA_GCCOLLECT, 0); + return mLua.getTotalMemoryUsage(); + }; + int64_t memWithScript; + { + sol::object s = mLua.runInNewSandbox("requireBig.lua"); + memWithScript = getMem(); + } + for (int i = 0; i < 100; ++i) // run many times to make small memory leaks visible + mLua.runInNewSandbox("requireBig.lua"); + int64_t memWithoutScript = getMem(); + // At this moment all instances of the script should be garbage-collected. + EXPECT_LT(memWithoutScript, memWithScript); + } } From 7b3aa621a45d6fcd31aff8e0b8921ecf46ab26ef Mon Sep 17 00:00:00 2001 From: Petr Mikheev Date: Sun, 26 Feb 2023 10:55:27 +0100 Subject: [PATCH 3/3] [Lua] Fix memory leak in sandboxed "require". --- components/lua/luastate.cpp | 40 ++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/components/lua/luastate.cpp b/components/lua/luastate.cpp index 9af617020e..453e9d1586 100644 --- a/components/lua/luastate.cpp +++ b/components/lua/luastate.cpp @@ -183,6 +183,13 @@ namespace LuaUtil mSol["writeToLog"] = [](std::string_view s) { Log(Debug::Level::Info) << s; }; + mSol["setEnvironment"] + = [](const sol::environment& env, const sol::function& fn) { sol::set_environment(env, fn); }; + mSol["loadFromVFS"] = [this](std::string_view packageName) { + return loadScriptAndCache(packageNameToVfsPath(packageName, mVFS)); + }; + mSol["loadInternalLib"] = [this](std::string_view packageName) { return loadInternalLib(packageName); }; + // Some fixes for compatibility between different Lua versions if (mSol["unpack"] == sol::nil) mSol["unpack"] = mSol["table"]["unpack"]; @@ -208,6 +215,19 @@ namespace LuaUtil end printGen = function(name) return function(...) return printToLog(name, ...) end end + function requireGen(env, loaded, loadFn) + return function(packageName) + local p = loaded[packageName] + if p == nil then + local loader = loadFn(packageName) + setEnvironment(env, loader) + p = loader(packageName) + loaded[packageName] = p + end + return p + end + end + function createStrictIndexFn(tbl) return function(_, key) local res = tbl[key] @@ -327,16 +347,7 @@ namespace LuaUtil loaded[key] = maybeRunLoader(value); for (const auto& [key, value] : packages) loaded[key] = maybeRunLoader(value); - env["require"] = [this, env, loaded, hiddenData](std::string_view packageName) mutable { - sol::object package = loaded[packageName]; - if (package != sol::nil) - return package; - sol::protected_function packageLoader = loadScriptAndCache(packageNameToVfsPath(packageName, mVFS)); - sol::set_environment(env, packageLoader); - package = call(packageLoader, packageName); - loaded[packageName] = package; - return package; - }; + env["require"] = mSol["requireGen"](env, loaded, mSol["loadFromVFS"]); sol::set_environment(env, script); return call(scriptId, script); @@ -348,14 +359,7 @@ namespace LuaUtil sol::table loaded(mSol, sol::create); for (const std::string& s : safePackages) loaded[s] = static_cast(mSandboxEnv[s]); - env["require"] = [this, loaded, env](const std::string& module) mutable { - if (loaded[module] != sol::nil) - return loaded[module]; - sol::protected_function initializer = loadInternalLib(module); - sol::set_environment(env, initializer); - loaded[module] = call({}, initializer, module); - return loaded[module]; - }; + env["require"] = mSol["requireGen"](env, loaded, mSol["loadInternalLib"]); return env; }