-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
first attempt at clearing npc events(callbacks) from scriptinterface … #3553
base: master
Are you sure you want to change the base?
first attempt at clearing npc events(callbacks) from scriptinterface … #3553
Conversation
I still need to find a way to add this kinda of code to MonsterInfo so we can prevent the leak for monsters events too. |
@@ -1110,6 +1112,14 @@ NpcEventsHandler::NpcEventsHandler(const std::string& file, Npc* npc) : | |||
} | |||
} | |||
|
|||
NpcEventsHandler::~NpcEventsHandler() | |||
{ | |||
std::array<int32_t, 7> eventArray = {creatureSayEvent, creatureDisappearEvent, creatureAppearEvent, creatureMoveEvent, playerCloseChannelEvent, playerEndTradeEvent, thinkEvent}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nekiro a static in the destructor means it will always try to delete the events of the first NpcEventsHandler to ever be deleted.
No need for the temporary though
for (int32_t eventId : {creatureSayEvent, creatureDisappearEvent, creatureAppearEvent, creatureMoveEvent, playerCloseChannelEvent, playerEndTradeEvent, thinkEvent}) {
I used an empty NPC script file with these contents function onCreatureAppear(cid)
end
function onCreatureDisappear(cid)
end
function onCreatureSay(cid, type, msg)
end
function onThink()
end And local bigTable = {}
local index = 1
for i = 1, 10000 do
local tbl = {}
local tmpIndex = 1
for i = 1, 100 do
tbl[tmpIndex] = ("%s%u%s%u"):format(("a"):rep(100), math.random(1000000000000), ("b"):rep(100), math.random(1000000000000))
tmpIndex = tmpIndex + 1
end
bigTable[index] = tbl
index = index + 1
end I then ran the test from #3552 again, with 5000 iterations each, totaling 15k NPC's. This is the memory graph The second tiny dip in the graph is due to me running |
New findings: it seems like Lua is reserving a certain amount of memory that it then re-uses, only allocating more when there is no more free room. With this PRCallbacks are unreferenced, memory usage stops increasing, GC times stay consistent - 690MB~ of memory used by the process after running the test 6x with 15k NPC's eachWithout this PRCallbacks stay referenced forever, memory usage keeps increasing, GC times increase as well 1.5GB~ of memory used by the process after running the test 4x with 15k NPC's eachNow we just need to unreference all the other places where |
… Combat:getTargets() where each call adds a new callback
scriptInterface->deleteEvent(scriptId); | ||
} | ||
} catch (const std::exception& e) { | ||
fmt::printf("CallBack::~CallBack(): its look like we have a dangling scriptInterface in a combat callback."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple std::cout
should be sufficient here since we aren't formatting anything, then we don't need to include the printf header from the fmt lib.
Correction of spelling in the message: It looks like we have a dangling scriptInterface in a combat callback.
…moved eventIds added eventIdPool to LuaScriptInterface so we don't waste already allocated indexes in events table(eventTableRef)
scriptInterface->deleteEvent(scriptId); | ||
} | ||
} catch (const std::exception& e) { | ||
fmt::printf("CallBack::~CallBack(): its look like we have a dangling scriptInterface in a combat callback."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt::printf("CallBack::~CallBack(): its look like we have a dangling scriptInterface in a combat callback."); | |
std::cout << "CallBack::~CallBack(): It looks like we have a dangling scriptInterface in a combat callback." << std::endl; |
@@ -23,6 +23,7 @@ | |||
|
|||
#include "pugicast.h" | |||
#include "tools.h" | |||
#include "fmt/printf.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "fmt/printf.h" |
if (scriptInterface && scriptId) { | ||
scriptInterface->deleteEvent(scriptId); | ||
} | ||
} catch (const std::exception& e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'e': unreferenced local variable
…lua events table
Pull Request Prelude
Changes Proposed
Remove/clear the events in the events table so resources can be freed on the lua side.
Issues addressed:
#3552