Thread Tools Display Modes
07-06-19, 04:03 PM   #1
doofus
A Chromatic Dragonspawn
Join Date: Feb 2018
Posts: 158
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?
  Reply With Quote
07-06-19, 04:17 PM   #2
Terenna
A Flamescale Wyrmkin
AddOn Author - Click to view addons
Join Date: Jun 2016
Posts: 105
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.
  Reply With Quote
07-06-19, 05:04 PM   #3
doofus
A Chromatic Dragonspawn
Join Date: Feb 2018
Posts: 158
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...

Last edited by doofus : 07-06-19 at 05:32 PM.
  Reply With Quote
07-06-19, 06:05 PM   #4
Kanegasi
A Molten Giant
 
Kanegasi's Avatar
AddOn Author - Click to view addons
Join Date: Apr 2007
Posts: 666
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.
  Reply With Quote
07-06-19, 06:32 PM   #5
jeruku
A Cobalt Mageweaver
 
jeruku's Avatar
AddOn Author - Click to view addons
Join Date: Oct 2010
Posts: 223
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
__________________
"I have not failed, I simply found 10,000 ways that did not work." - Thomas Edison

Last edited by jeruku : 07-06-19 at 06:45 PM. Reason: I never feel like I convey my messages right.
  Reply With Quote
07-07-19, 12:39 AM   #6
doofus
A Chromatic Dragonspawn
Join Date: Feb 2018
Posts: 158
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.

Last edited by doofus : 07-07-19 at 01:43 AM.
  Reply With Quote
07-07-19, 02:09 AM   #7
SDPhantom
A Pyroguard Emberseer
 
SDPhantom's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2006
Posts: 2,313
Originally Posted by jeruku View Post
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.
__________________
WoWInterface AddOns
"All I want is a pretty girl, a decent meal, and the right to shoot lightning at fools."
-Anders (Dragon Age: Origins - Awakening)
  Reply With Quote
07-07-19, 08:14 AM   #8
Kanegasi
A Molten Giant
 
Kanegasi's Avatar
AddOn Author - Click to view addons
Join Date: Apr 2007
Posts: 666
Originally Posted by doofus View Post
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?
  Reply With Quote
07-07-19, 11:54 AM   #9
Seerah
Fishing Trainer
 
Seerah's Avatar
WoWInterface Super Mod
Featured
Join Date: Oct 2006
Posts: 10,860
Why don't you just post your /real/ code?
__________________
"You'd be surprised how many people violate this simple principle every day of their lives and try to fit square pegs into round holes, ignoring the clear reality that Things Are As They Are." -Benjamin Hoff, The Tao of Pooh

  Reply With Quote
07-07-19, 12:53 PM   #10
SDPhantom
A Pyroguard Emberseer
 
SDPhantom's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2006
Posts: 2,313
Originally Posted by doofus View Post
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.



Originally Posted by doofus View Post
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.
__________________
WoWInterface AddOns
"All I want is a pretty girl, a decent meal, and the right to shoot lightning at fools."
-Anders (Dragon Age: Origins - Awakening)

Last edited by SDPhantom : 07-07-19 at 01:10 PM.
  Reply With Quote
07-07-19, 03:15 PM   #11
jeruku
A Cobalt Mageweaver
 
jeruku's Avatar
AddOn Author - Click to view addons
Join Date: Oct 2010
Posts: 223
Originally Posted by Seerah View Post
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.

Originally Posted by SDPhantom View Post
"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.
__________________
"I have not failed, I simply found 10,000 ways that did not work." - Thomas Edison
  Reply With Quote
07-07-19, 03:29 PM   #12
Fizzlemizz
I did that?
 
Fizzlemizz's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Dec 2011
Posts: 1,871
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.
__________________
Fizzlemizz
Maintainer of Discord Unit Frames and Discord Art.
Author of FauxMazzle, FauxMazzleHUD and Move Pad Plus.

Last edited by Fizzlemizz : 07-07-19 at 03:33 PM.
  Reply With Quote
07-07-19, 04:30 PM   #13
SDPhantom
A Pyroguard Emberseer
 
SDPhantom's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2006
Posts: 2,313
Originally Posted by Fizzlemizz View Post
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.
__________________
WoWInterface AddOns
"All I want is a pretty girl, a decent meal, and the right to shoot lightning at fools."
-Anders (Dragon Age: Origins - Awakening)
  Reply With Quote
07-07-19, 05:02 PM   #14
Fizzlemizz
I did that?
 
Fizzlemizz's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Dec 2011
Posts: 1,871
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".
__________________
Fizzlemizz
Maintainer of Discord Unit Frames and Discord Art.
Author of FauxMazzle, FauxMazzleHUD and Move Pad Plus.
  Reply With Quote
07-07-19, 08:54 PM   #15
SDPhantom
A Pyroguard Emberseer
 
SDPhantom's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2006
Posts: 2,313
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.
__________________
WoWInterface AddOns
"All I want is a pretty girl, a decent meal, and the right to shoot lightning at fools."
-Anders (Dragon Age: Origins - Awakening)

Last edited by SDPhantom : 07-07-19 at 08:58 PM.
  Reply With Quote
07-07-19, 08:55 PM   #16
Fizzlemizz
I did that?
 
Fizzlemizz's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Dec 2011
Posts: 1,871
Many thanks for the confirmation.
__________________
Fizzlemizz
Maintainer of Discord Unit Frames and Discord Art.
Author of FauxMazzle, FauxMazzleHUD and Move Pad Plus.
  Reply With Quote
07-08-19, 07:56 AM   #17
doofus
A Chromatic Dragonspawn
Join Date: Feb 2018
Posts: 158
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.
  Reply With Quote
07-08-19, 09:05 AM   #18
Ketho
A Pyroguard Emberseer
 
Ketho's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2010
Posts: 1,026
This is why you should post your code in the first place
  Reply With Quote
07-08-19, 11:29 AM   #19
DashingSplash
A Murloc Raider
Join Date: Jun 2019
Posts: 7
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?
  Reply With Quote
07-08-19, 04:00 PM   #20
Terenna
A Flamescale Wyrmkin
AddOn Author - Click to view addons
Join Date: Jun 2016
Posts: 105
Originally Posted by Terenna View Post
A better solution would be to post your code and allow the problem to be solved objectively rather than through guesses.
Originally Posted by Ketho View Post
This is why you should post your code in the first place
Almost 48 hours and countless times typing /reload
  Reply With Quote

WoWInterface » Developer Discussions » Lua/XML Help » My favourite addon leaks memory

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off