WoWInterface

WoWInterface (https://www.wowinterface.com/forums/index.php)
-   Lua/XML Help (https://www.wowinterface.com/forums/forumdisplay.php?f=16)
-   -   My favourite addon leaks memory (https://www.wowinterface.com/forums/showthread.php?t=57263)

doofus 07-06-19 04:03 PM

My favourite addon leaks memory
 
So I have realised that my favourite addon (which I wrote) is misbehaving. Wow, slowly gets worse and worse, until it slows to a crawl. A /reload fixes it until a little while later it does it again.

So I looked at the ? (Game Menu) and it shows the addon steadily increasing its memory while in combat. It starts at a modest 650K, but after some time it goes to many MBs.

I presume this is related to the general issue I am facing.

Before I start hacking the code apart, have they recently made any changes that might have caused this?

Terenna 07-06-19 04:17 PM

We could rattle off all the changes they made. And that may or may not click something in your brain as to the problem. But that assumes that this issue wasn't present before the patch, which may or may not be true. A better solution would be to post your code and allow the problem to be solved objectively rather than through guesses.

doofus 07-06-19 05:04 PM

OK, ignore all this.

a lua file has

...
...
...
local BA_Data1 = { };

...
...
...

On some event

local function someevent()

BA_Data1["AAAA1"] = {};

end

That leaks - I am not sure why...

Kanegasi 07-06-19 06:05 PM

That's just Lua. When you create a new table and assign it to that key, the previous table, despite being empty, still exists. It's just unobtainable, floating in memory, until it gets released by garbage collection. This has been a known issue for years. Blizzard relatively recently developed "table pools" where they reuse tables, even empty tables.

Instead of assigning a new empty table to an object that already has a table, use wipe() instead. If your desired effect is to just make sure the object is a table, use type() before assigning it a table.

jeruku 07-06-19 06:32 PM

What Kanegasi said. And you can't always rely on garbage collection for things like strings either, especially during events that trigger much like OnUpdate. Though, garbage collection is slightly more forgiving for strings... if only a little. Addons should have an initialization phase for every table where it's created and at any point you need to reset/reuse, or wipe, it you should use table.wipe.

I'm not sure how they're using the table but a reference/key can be read either as a string or a variable.
Lua Code:
  1. BA_Data1["AAAA1"] == BA_Data1.AAAA1
So
Lua Code:
  1. --upvalue table.wipe for ease of use and performance
  2. local wipe = table.wipe
  3.  
  4. local BA_Data1 = {}  -- should be the only place you set it as a table
  5.  
  6. -- stuff happens
  7.  
  8. BA_Data1.AAAA1 = wipe(BA_Data1.AAAA1) -- that should solve the memory issue

doofus 07-07-19 12:39 AM

I was 99.9% sure it was bad GC, and I wrote a big message yesterday, but then I re-wrote the whole message to make it simpler and forgot to mention the GC in the new edit.

OK so after some experimentation, I recap:

1) My addon leaks memory like a sieve. I presume when it has leaked enough it slows down WoW because it works on screen update, ie very often, and probably with large memory lying about some operations become slower

2) This manifests itself that we go into the raid, all is well, but by the 5th wipe on Rastakhan my screen slows to a crawl, effective 2-3 FPS

3) I discovered that a /reload fixes the issue, magically

4) You told me that my tables are all leaking, every time I do " local myTable = { } ". You suggested to use wipe().

5) I tried to use wipe() but it failed because it does not help with tables inside tables. It also makes the code cumbersome and weird to read.

6) but we can force GC with " collectgarbage(); " - this seems to work beautifully.

Now I am trying to find the best place to put this. From my experience 100 years ago, I remember that you don't want to be calling GC because it blocks all threads and can halt your code for a while. Actually I know of a very, very famous stock exchange, that decided to re-write their code in .. .net, and they had disabled GC for this reason. The exchange needed to be rebooted every evening to reclaim memory.

So, back to WoW, I am slighly worried that if I call GC, it might affect ALL addons, and it might block all addons and threads, just when I am trying to taunt Bwonsamdi or cast Guardian Angel to the tank...

Currently I have added GC during (a) GCD (b) casting (c) not in combat, hoping it should not affect things too much.

SDPhantom 07-07-19 02:09 AM

Quote:

Originally Posted by jeruku (Post 332762)
Though, garbage collection is slightly more forgiving for strings... if only a little.

Strings in general are strange in Lua. They're stored by a hash of their content. So two strings of equal content always point to the same object in memory.

Concatenation, while it seems innocuous, is actually the biggest issue with memory bloat when it comes to strings. With each concatenation operation, you add a copy of said string with whatever piece you appended to it.

Example:
Code:

print("some".." ".."sample".." ".."text")
To break this down, each segment is its own string, so we start out with 4 (remember, the spaces are the same string object since they are equal). Each concatenation step adds a new string to memory containing the result of the operation. After this is done, we have 4 more strings in memory. This means this one line created 8 strings, the last of which is the completed expression that gets printed out.

Another big issue with strings is formatting them with numbers. Each time you format a string with numbers, each unique combination of numbers produces a new string object.

Kanegasi 07-07-19 08:14 AM

Quote:

Originally Posted by doofus (Post 332773)
I tried to use wipe() but it failed because it does not help with tables inside tables.

I don't understand this part. What do you mean "tables inside tables"? Is wipe not letting you clean that table which happens to be in a table or do you have nested tables inside the table you're trying to clean? As for the fail part, are you getting errors? What happened when you used wipe?

Seerah 07-07-19 11:54 AM

Why don't you just post your /real/ code? ;)

SDPhantom 07-07-19 12:53 PM

Quote:

Originally Posted by doofus (Post 332773)
5) I tried to use wipe() but it failed because it does not help with tables inside tables.

Here's a drycoded example of table.wipe() that runs recursively and preserves links to nested tables.
Lua Code:
  1. local NestSafeWipe; do--    function NestSafeWipe(tbl)
  2. --  Local References (finds these faster than calling from global)
  3.     local pairs=pairs;
  4.     local table_insert=table.insert;
  5.     local table_remove=table.remove;
  6.     local table_wipe=table.wipe;
  7.     local type=type;
  8.  
  9. --  Local Variables (mitigates memory usage if used often)
  10.     local SeenTables={};--  Helps keep track of recursion and looped references (don't want to clean what we've alreay cleaned, also prevents infinite loops)
  11.     local SavedLinks={};
  12.     local TableQueue={};
  13.  
  14.     function NestSafeWipe(tbl)
  15.         SeenTables[tbl]=true;-- Mark table as "seen"
  16.         table_insert(TableQueue,tbl);-- Add current table to queue (to initiate loop)
  17.  
  18.         repeat
  19.             local curtbl=table_remove(TableQueue);--    Fetch from queue (doesn't matter which end)
  20.  
  21.             for key,val in pairs(curtbl) do--   Find nested tables
  22.                 if type(val)=="table" then
  23.                     SavedLinks[key]=val;--  Save key and table
  24.                     if not SeenTables[val] then--   Queue if not "seen"
  25.                         SeenTables[val]=true;-- Mark as "seen"
  26.                         table_insert(TableQueue,val);-- Add to queue
  27.                     end
  28.                 end
  29.             end
  30.  
  31.             table_wipe(curtbl);--   Wipe current table
  32.             for key,val in pairs(SavedLinks) do curtbl[key]=val; end--  Restore nested tables
  33.             table_wipe(SavedLinks)--    Done with this data
  34.         until #TableQueue<=0--  Exits when queue is empty
  35.         table_wipe(SeenTables);--   Done with this data
  36.     end
  37. end
Note: The outer do...end is used to explicitly define a variable scope to prevent accidental tampering with the function's persistent variables. To allow the function's presence outside this scope, the function prototype is defined immediately preceding it.



Quote:

Originally Posted by doofus (Post 332773)
It also makes the code cumbersome and weird to read.

Think of the languages that don't have a GC and require complex memory structures to be allocated and freed manually. "Just run the GC more" is a poor excuse for not optimizing your code.

jeruku 07-07-19 03:15 PM

Quote:

Originally Posted by Seerah (Post 332783)
Why don't you just post your /real/ code? ;)

This is preferable as anyone can check it for any other errors or give some helpful advice.

Quote:

Originally Posted by SDPhantom (Post 332791)
"Just run the GC more" is a poor excuse for not optimizing your code.

Agreed, I started out back when Blizzard didn't have events for nameplates and can tell you now that relying on garbage collection is still the most common misconception. Best practice is to reserve concatenation for creating unique table/frame names using a nested table/library for actual use; and as far as I know tables/frames are never deleted so recycle, reuse, and re-purpose.

Fizzlemizz 07-07-19 03:29 PM

I thought Blizzard indicated that tables did get picked up by GC when the coordinates functions(s) changed to return a table instead of x, y. Could also be I misunderstood.

SDPhantom 07-07-19 04:30 PM

Quote:

Originally Posted by Fizzlemizz (Post 332798)
I thought Blizzard indicated that tables did get picked up by GC

They do, but the issue is the performance degradation of triggering the GC too frequently with way too much garbage.

Fizzlemizz 07-07-19 05:02 PM

I wasn't saying garbage collection shouldn't be minimised just making sure I had my understanding about what it does/doesn't collect is correct. I presume, as jeruku said, frames are still not "collected".

SDPhantom 07-07-19 08:54 PM

Frames are a special occasion. The table portion has a reference stored in C code, so that doesn't get released to the GC. Not to mention the C side resources that aren't even exposed to Lua.

Fizzlemizz 07-07-19 08:55 PM

Many thanks for the confirmation.

doofus 07-08-19 07:56 AM

OK I have traced deeper into the problem and have better news (for me).

To recap:

WoW was getting progressively slow, to the point of crawling to a halt. It took a couple of hours to reach that point, two hours of trying Rastakhan on Heroic to be precise.

I discovered that a /reload fixed it (after we wiped for the nth time).

I then started to find the culprit, which was my own addon.

I immediately noticed that my addon started at about 700K but during combat it would rise with no apparent let up. This could mean a lazy GC or something more sinister.

After reading your suggestions I realised that any kind of table assignment increases the reported memory (hover mouse over the ?). wipe() did not fix it, nor did using local tables (you'd think local variables get thrown away at the end of the function if no references to them).

I then discovered we can force GC with collectgarbage() and I used it liberally, and my memory consumption went down, however it made the screen jerky, so I removed it.

Finally I found the culprit as being not memory or lazy garbage collection, but my misuse of "CreateFrame", as in
gameTooltip = CreateFrame("GameTooltip", "BA_GameTooltip", UIParent, "GameTooltipTemplate");

which I used to parse the tooltip of Azerite gear in order to find the available traits. 3-4 times a second. This is what causes the screen jerkiness and eventually the slowdown to a crawl. I read on the wiki/gamepedia that you must re-use frames, they cannot be deleted! So I did just this, and all seems to be good.

Memory still seems to be going up, but it was not the reason for the jerkiness and slowdown, it was all those frames I was creating.

Ketho 07-08-19 09:05 AM

This is why you should post your code in the first place

DashingSplash 07-08-19 11:29 AM

I got a bit intrigued by the table wipe discussion since I used it for my first AddOn that I made (or rather an early version of my first AddOn). Since I had trouble with the tables keeping their old values when the UPDATE_BINDINGS event was triggered I had to wipe the tables as shown below.

Lua Code:
  1. local table_1 = {t = {}, st = {}, ct = {}, at = {}} --Filled after dashingUpdate has executed
  2. local table_2 = {t2 = {}, st2 = {}, ct2 = {}, at2 = {}} --Filled after dashingUpdate has executed
  3.  
  4. local function dashingUpdate(self, event, ...)
  5.     for _, tableWipe in pairs(table_1) do
  6.         wipe(tableWipe)
  7.     end
  8.    
  9.     for _, tableWipe in pairs(table_2) do
  10.         wipe(tableWipe)
  11.     end
  12. end

Would this actually be an OK way to do it, or would I get problems if the addon had been updating a lot more?

Terenna 07-08-19 04:00 PM

Quote:

Originally Posted by Terenna (Post 332758)
A better solution would be to post your code and allow the problem to be solved objectively rather than through guesses.

Quote:

Originally Posted by Ketho (Post 332810)
This is why you should post your code in the first place

Almost 48 hours and countless times typing /reload


All times are GMT -6. The time now is 08:25 PM.

vBulletin © 2019, Jelsoft Enterprises Ltd
© 2004 - 2019 MMOUI