hooksecurefunc & memory use

[Aprikot]

Hello, I recently made a button border AddOn, the first version of which took this approach:

• main function (create border & apply settings)
  • if object is regular action button do stuff
  • if object is special action button do other stuff
  • if object is aura button do other other stuff
• hooksecurefunc 1
  • for all objects in group A do main function
• hooksecurefunc 2
  • for all objects in group B do main function
• hooksecurefunc 3
  • for all objects in group C do main function

This "worked", the trouble was sporadic memory usage spikes & consequent FPS drops...usually when doing certain things (like shapshifting). I would run through Elwynn doing nothing but stealthing/unstealthing, and would drop to zero FPS both ways for a second as this AddOn's memory use grew from 10 KB to 2 MB momentarily (no other AddOns loaded).

My second version took an approach I've seen in a few other AddOns (notably nMainbar):

• hooksecurefunc 1
  • create border & apply settings
• hooksecurefunc 2
  • create border & apply settings
• hooksecurefunc 3
  • create border & apply settings

The result is dramatically better performance with no lag issues whatsoever. I'm trying to understand why, as I still like the idea of one "do stuff" function that is referenced in all subsequent hooked secure functions.

first version | second version

[Torhal]

If you're creating textures and frames every time DoStuff() is called, expanded memory usage is to be expected. If, however, you create the textures and frames exactly once and apply them when they're needed, you're only ever growing the memory once.

Without seeing code, I can't rightly say what's happening.

[Aprikot]

I have pastebin links in the op, but here's the meat of the two versions:

First version:

local BorderBeautification = function(object)
       
                if IsAddOnLoaded("FacePaint") and FacePaint then
                        tc = {r = topcolor.r, g = topcolor.g, b = topcolor.b, a = topalpha}
                        bc = {r = bottomcolor.r, g = bottomcolor.g, b = bottomcolor.b, a = bottomalpha}
                else
                        tc = {r = BorderColors.top.r, g = BorderColors.top.g, b = BorderColors.top.b, a = BorderColors.top.a}
                        bc = {r = BorderColors.bot.r, g = BorderColors.bot.g, b = BorderColors.bot.b, a = BorderColors.bot.a}
                end
       
                -- Make border texture
                for number = 1, 12 do
                        local button = _G[object..number];
                        local icon = _G[object..number.."Icon"];
                        if button then
                                button:SetNormalTexture("Interface\\BUTTONS\\UI-Quickslot2")
                        end
                        local border
                       
                        -- Shapeshift & Pet bars
                        if (object == "ShapeshiftButton" or object == "PetActionButton") then
                                border = _G[object..number.."NormalTexture2"]
                                if border then 
                                        border:ClearAllPoints()
                                        border:SetPoint("TOPLEFT", button, -15, 15)
                                        border:SetPoint("BOTTOMRIGHT", button, 15, -15)
                                        border:SetGradientAlpha("VERTICAL", bc.r, bc.g, bc.b, ba, tc.r, tc.g, tc.b, ta)
                                end
                                if icon then
                                        icon:SetTexCoord(.05, .95, .05, .95);
                                        icon:SetPoint("TOPLEFT", button, -1, 1);
                                        icon:SetPoint("BOTTOMRIGHT", button, 1, -1);
                                end
                               
                        -- Totem bar                           
                        elseif (object == "MultiCastActionButton") then
                                border = _G[object..number.."NormalTexture"]
                                if border then 
                                        border:ClearAllPoints()
                                        border:SetPoint("TOPLEFT", button, -15, 15)
                                        border:SetPoint("BOTTOMRIGHT", button, 15, -15)
                                        border:SetGradientAlpha("VERTICAL", bc.r, bc.g, bc.b, ba, tc.r, tc.g, tc.b, ta)
                                end
                                if icon then
                                        icon:SetTexCoord(.05, .95, .05, .95);
                                        icon:SetPoint("TOPLEFT", button, 1, -1);
                                        icon:SetPoint("BOTTOMRIGHT", button, -1, 1);
                                end
                               
                        -- Action bars
                        else
                                border = _G[object..number.."NormalTexture"]
                                if border then 
                                        border:ClearAllPoints()
                                        border:SetPoint("TOPLEFT", button, -14, 14)
                                        border:SetPoint("BOTTOMRIGHT", button, 14, -14)
                                        border:SetGradientAlpha("VERTICAL", bc.r, bc.g, bc.b, ba, tc.r, tc.g, tc.b, ta)
                                end
                                if icon then
                                        icon:SetTexCoord(.05, .95, .05, .95);
                                        icon:SetPoint("TOPLEFT", button, 0, 0);
                                        icon:SetPoint("BOTTOMRIGHT", button, 0, 0);
                                end                            
                        end
                end
        end
       
--      McBlastOff
        ----------
       
        hooksecurefunc("ActionButton_Update", function(self)
                for i, v in pairs({"ActionButton", "MultiBarBottomLeftButton", "MultiBarBottomRightButton", "MultiBarLeftButton", "MultiBarRightButton", "MultiCastActionButton", "MultiCastSummonSpellButton", "MultiCastRecallSpellButton"}) do
                        BorderBeautification(v)
                end
        end);
       
        hooksecurefunc("ActionButton_UpdateUsable", function(self)
                for i, v in pairs({"ActionButton", "MultiBarBottomLeftButton", "MultiBarBottomRightButton", "MultiBarLeftButton", "MultiBarRightButton", "MultiCastActionButton", "MultiCastSummonSpellButton", "MultiCastRecallSpellButton"}) do
                        BorderBeautification(v)
                end
        end);
 
        hooksecurefunc("ActionButton_ShowGrid", function(self)
                for i, v in pairs({"ActionButton", "MultiBarBottomLeftButton", "MultiBarBottomRightButton", "MultiBarLeftButton", "MultiBarRightButton", "MultiCastActionButton", "MultiCastSummonSpellButton", "MultiCastRecallSpellButton"}) do
                        BorderBeautification(v)
                end
        end);
       
        hooksecurefunc("PetActionBar_Update",  function(self)
                for i, v in pairs({"PetActionButton", "ShapeshiftButton"}) do
                        BorderBeautification(v)
                end
        end);

Second version:

local colorit = function(object)
                if IsAddOnLoaded("FacePaint") and FacePaint.enable and not FacePaint.invert then
                        tc = {r = topcolor.r, g = topcolor.g, b = topcolor.b, a = topalpha}
                        bc = {r = bottomcolor.r, g = bottomcolor.g, b = bottomcolor.b, a = bottomalpha}
                elseif IsAddOnLoaded("FacePaint") and FacePaint.enable and FacePaint.invert then
                        tc = {r = bottomcolor.r, g = bottomcolor.g, b = bottomcolor.b, a = bottomalpha}
                        bc = {r = topcolor.r, g = topcolor.g, b = topcolor.b, a = topalpha}
                else
                        tc = {r = bordercolor.top.r, g = bordercolor.top.g, b = bordercolor.top.b, a = bordercolor.top.a}
                        bc = {r = bordercolor.bot.r, g = bordercolor.bot.g, b = bordercolor.bot.b, a = bordercolor.bot.a}
                end
                object:SetGradientAlpha("VERTICAL", bc.r, bc.g, bc.b, bc.a, tc.r, tc.g, tc.b, tc.a)
        end
       
        hooksecurefunc("ActionButton_Update", function(self)
                local button = _G[self:GetName()]
                local icon = _G[self:GetName().."Icon"]
                local border = _G[self:GetName().."NormalTexture"]
                button:SetNormalTexture("Interface\\BUTTONS\\UI-Quickslot2")
                icon:SetTexCoord(.05, .95, .05, .95);
                icon:SetPoint("TOPLEFT", button, 0, 0);
                icon:SetPoint("BOTTOMRIGHT", button, 0, 0);
                border:ClearAllPoints()
                border:SetPoint("TOPLEFT", button, -14, 14)
                border:SetPoint("BOTTOMRIGHT", button, 14, -14)
                colorit(border)
        end);
       
        hooksecurefunc("ActionButton_UpdateUsable", function(self)
                colorit(_G[self:GetName().."NormalTexture"])
        end);
       
        hooksecurefunc("ActionButton_ShowGrid", function(self)
                colorit(_G[self:GetName().."NormalTexture"])
        end);
       
        hooksecurefunc("PetActionBar_Update",  function()
                for i, object in pairs({"PetActionButton", "ShapeshiftButton", "PossessButton"}) do
                        for number = 1, 12 do
                                local button = _G[object..number];
                                local icon = _G[object..number.."Icon"];
                                if button then
                                        button:SetNormalTexture("Interface\\BUTTONS\\UI-Quickslot2")
                                end
                                local border
                                if (object == "ShapeshiftButton" or object == "PetActionButton") then
                                        border = _G[object..number.."NormalTexture2"]
                                        if border then 
                                                border:ClearAllPoints()
                                                border:SetPoint("TOPLEFT", button, -15, 15)
                                                border:SetPoint("BOTTOMRIGHT", button, 15, -15)
                                                colorit(border)
                                        end
                                        if icon then
                                                icon:SetTexCoord(.05, .95, .05, .95);
                                                icon:SetPoint("TOPLEFT", button, -1, 1);
                                                icon:SetPoint("BOTTOMRIGHT", button, 1, -1);
                                        end            
                                else
                                        border = _G[object..number.."NormalTexture"]
                                        if border then 
                                                border:ClearAllPoints()
                                                border:SetPoint("TOPLEFT", button, -14, 14)
                                                border:SetPoint("BOTTOMRIGHT", button, 14, -14)
                                                colorit(border)
                                        end
                                        if icon then
                                                icon:SetTexCoord(.05, .95, .05, .95);
                                                icon:SetPoint("TOPLEFT", button, 0, 0);
                                                icon:SetPoint("BOTTOMRIGHT", button, 0, 0);
                                        end                            
                                end
                        end
                end
        end);
 
        hooksecurefunc("AuraButton_Update", function(self, index)
                if auraborder then
                        local button = _G[self..index]
                        local icon = _G[self..index.."Icon"]
                        local border = _G[self..index.."Border"]
                        if icon then
                                icon:SetTexCoord(.07, .93, .07, .93);
                                --icon:SetPoint("TOPLEFT", button, -1, 1);
                                --icon:SetPoint("BOTTOMRIGHT", button, 1, -1);
                        end
                        if border then
                                border:SetTexture("Interface\\BUTTONS\\UI-Quickslot2")
                                border:ClearAllPoints()
                                border:SetPoint("TOPLEFT", button, -12, 12)
                                border:SetPoint("BOTTOMRIGHT", button, 12, -12)
                                border:SetTexCoord(0, 1, 0, 1)
                                colorit(border)
                        end
                        if button and not border then
                                newborder = button:CreateTexture("$parentOverlay", "ARTWORK")
                                newborder:SetParent(button)
                                newborder:SetTexture("Interface\\BUTTONS\\UI-Quickslot2")
                                newborder:SetPoint("TOPLEFT", button, -12, 12)
                                newborder:SetPoint("BOTTOMRIGHT", button, 12, -12)
                                colorit(newborder)
                        end
                end
        end);
 
        for i = 1, 3 do
                local button = _G["TempEnchant"..i]
                local icon = _G["TempEnchant"..i.."Icon"]
                icon:SetTexCoord(.07, .93, .07, .93);
                --icon:SetPoint("TOPLEFT", button, 0, 0);
                --icon:SetPoint("BOTTOMRIGHT", button, 0, 0);
                local border = _G["TempEnchant"..i.."Border"]
                border:SetTexture("Interface\\BUTTONS\\UI-Quickslot2")
                border:ClearAllPoints()
                border:SetPoint("TOPLEFT", button, -12, 12)
                border:SetPoint("BOTTOMRIGHT", button, 12, -12)
                border:SetTexCoord(0, 1, 0, 1)
                colorit(border)
        end

[Torhal]

Ah - I missed the links in the OP...I must have thought it was part of a sig.

Anyway...it looks like the original method iterates over everything every time one of the hooked functions is called, whereas the new method specifically targets the appropriate UI widget.

If the hooked functions are being called many times in a short period, the excessive iterations (and thereby all of the function calls per iteration) will definitely have a huge impact on your FPS, whereas the specific targeting may still have minor performance issues (if not throttled) but definitely nothing of the magnitude of the "let's change everything" approach.

[Aprikot]

I guess I'm trying to determine if the first approach is fundementally unsound, or just poorly implemented.

The content of my "main" function is all conditional on what objects are being passed to it, and in my hooksecurefunc calls I'm trying to say "hey go check that main function, and if it knows who you are, do what it tells you".

It seems like this would be fairly efficient resource-wise, but I'm largely ignorant of best practices & optimization (but learning ).

[Saiket]

Your original code creates at least three garbage tables every time one of the hooked functions is called. Some of those hooked functions, like ActionButton_UpdateUsable, get called a lot: once per visible action button for each action that becomes usable/unusable. Every action button is registered for ACTIONBAR_UPDATE_USABLE, and each one responds by calling ActionButton_UpdateUsable separately.



Imagine you have 20 action buttons visible, and 10 of them become unusable when you unstealth. If you unstealth, ACTIONBAR_UPDATE_USABLE fires 10 times, and ActionButton_UpdateUsable gets called 10*20 = 200 times. Then, your hook creates one garbage table each call:

hooksecurefunc("ActionButton_UpdateUsable", function(self)
                for i, v in pairs({"ActionButton", "MultiBarBottomLeftButton", "MultiBarBottomRightButton", "MultiBarLeftButton", "MultiBarRightButton", "MultiCastActionButton", "MultiCastSummonSpellButton", "MultiCastRecallSpellButton"}) do

Then, BorderBeautification gets called for each of those 8 object types, totaling 200*8 = 1600 calls. Each of those calls creates two more garbage tables for top and bottom color:

local BorderBeautification = function(object)
       
                if IsAddOnLoaded("FacePaint") and FacePaint then
                        tc = {r = topcolor.r, g = topcolor.g, b = topcolor.b, a = topalpha}
                        bc = {r = bottomcolor.r, g = bottomcolor.g, b = bottomcolor.b, a = bottomalpha}
                else
                        tc = {r = BorderColors.top.r, g = BorderColors.top.g, b = BorderColors.top.b, a = BorderColors.top.a}
                        bc = {r = BorderColors.bot.r, g = BorderColors.bot.g, b = BorderColors.bot.b, a = BorderColors.bot.a}
                end

In total, 3400 garbage tables created by unstealthing in this case--200 object list tables plus 1600*2 color tables.

That explains why your memory usage ballooned when you spammed stealth/unstealth. The other technique in your second version is more efficient because it doesn't recolor the same button multiple times per event like the first did. It still would still create a fair number of garbage color tables though.



I suggest using the preexisting topcolor/bottomcolor and BorderColors tables instead of copying them each time. Using the second version of your code, here's an updated colorit function that doesn't create any new tables:

local colorit = function(object)
    local tc, bc, ta, ba;
    if IsAddOnLoaded("FacePaint") and FacePaint.enable and not FacePaint.invert then
        tc, ta = topcolor, topalpha;
        bc, ba = bottomcolor, bottomalpha;
    elseif IsAddOnLoaded("FacePaint") and FacePaint.enable and FacePaint.invert then
        tc, ta = bottomcolor, bottomalpha;
        bc, ba = topcolor, topalpha;
    else
        tc, bc = bordercolor.top, bordercolor.bot;
        ta, ba = tc.a, bc.a;
    end
    object:SetGradientAlpha("VERTICAL", bc.r, bc.g, bc.b, ba, tc.r, tc.g, tc.b, ta)
end

[Aprikot]

@Saiket: very eyeopening & helpful (& humbling ), ty!

I think I've reread your post & reexamined my code x100 so far, and am nearing comprehension.

I'll be trying out the table-less colorit function. Somewhat related, I hadn't been able to capture/transfer color values from FacePaint's variables (topcolor, topalpha, bottomcolor, bottomalpha) without using global variables in both that AddOn and this. I'm now wondering if it was due to my use of tables, and I should probably revisit that in FacePaint.

In general is there an accepted practice of sharing varibles between AddOns (or at least one reading the other) which avoids the global namespace? Saved variables I'm guessing?

[Seerah]

Saved variables are in the global namespace.

[Saiket]

Originally Posted by Aprikot In general is there an accepted practice of sharing varibles between AddOns (or at least one reading the other) which avoids the global namespace? Saved variables I'm guessing?

Most addons have a main table they use for all of their storage, so that no matter how many variables and functions they use, the "namespace" main table only takes up one global. For example if FacePaint had one global table named FacePaint, you could save and access the color tables to/from FacePaint.topcolor and FacePaint.bottomcolor. You could also read the other mod's saved variables table as Seerah suggested.

[Aprikot]

Originally Posted by Seerah Saved variables are in the global namespace.

D'oh!

Originally Posted by Saiket Most addons have a main table they use for all of their storage, so that no matter how many variables and functions they use, the "namespace" main table only takes up one global. For example if FacePaint had one global table named FacePaint, you could save and access the color tables to/from FacePaint.topcolor and FacePaint.bottomcolor. You could also read the other mod's saved variables table as Seerah suggested.

I see, so would this:

-- FacePaint Config
local class = true
local gradient = true
topcolor = {
    r = 0.9,
    g = 0.9,
    b = 0.9,
}
bottomcolor = {
    r = 0.1,
    g = 0.1,
    b = 0.1,
}
topalpha = 1.0
bottomalpha = 1.0

become this?

-- FacePaint Config
FacePaintTable = {
    class = true
    gradient = true
    topcolor = {
        r = 0.9,
        g = 0.9,
        b = 0.9,
    }
    bottomcolor = {
        r = 0.1,
        g = 0.1,
        b = 0.1,
    }
    topalpha = 1.0
    bottomalpha = 1.0
}

I've been curious for awhile about namspace declarations at the beginning of AddOns (local _, ns = ...), but if I understand correctly, this is to use multiple Lua files under the same AddOn?

[Saiket]

Originally Posted by Aprikot ...snip... -- FacePaint Config FacePaintTable = {     class = true     gradient = true     topcolor = {         r = 0.9,         g = 0.9,         b = 0.9,     }     bottomcolor = {         r = 0.1,         g = 0.1,         b = 0.1,     }     topalpha = 1.0     bottomalpha = 1.0 } I've been curious for awhile about namspace declarations at the beginning of AddOns (local _, ns = ...), but if I understand correctly, this is to use multiple Lua files under the same AddOn?

Yep, the only thing special about that "ns" table is that it gets passed to each file. Your snippet above would work, or you could use the ns table instead.

I suggest creating "getter" functions instead though, to simplify how other addons interact with FacePaint. Basically, instead of requiring other mods to reach into the guts of FacePaint to get color values, FacePaint could provide a few standard, reliable functions to return color settings. If it had two functions, FacePaint.GetColorTop and FacePaint.GetColorBottom, they could handle the logic of when colors are flipped, or if class coloring is enabled, or any other feature FacePaint gets in the future. Here's one way you could set something like that up, with the ns table:

local ns = select(2, ...)
FacePaint = ns
 
 
-- FacePaint Config
local class = true
local gradient = true
local topcolor = {
    r = 0.9,
    g = 0.9,
    b = 0.9,
}
local bottomcolor = {
    r = 0.1,
    g = 0.1,
    b = 0.1,
}
local topalpha = 1.0
local bottomalpha = 1.0
 
 
function FacePaint.GetColorTop ()
    if FacePaint.enable then
        if FacePaint.invert then
            return bottomcolor.r, bottomcolor.g, bottomcolor.b, bottomtalpha
        else
            return topcolor.r, topcolor.g, topcolor.b, topalpha
        end
    end
end
function FacePaint.GetColorBottom ()
    if FacePaint.enable then
        if FacePaint.invert then
            return topcolor.r, topcolor.g, topcolor.b, topalpha
        else
            return bottomcolor.r, bottomcolor.g, bottomcolor.b, bottomalpha
        end
    end
end

If FacePaint were set up like this, your colorit function could be simplified:

local colorit = function(object)
    local br, bg, bb, ba, tr, tg, tb, ta
    if IsAddOnLoaded("FacePaint") and FacePaint
        and FacePaint.GetColorTop and FacePaint.GetColorTop()
    then -- Compatible version of FacePaint enabled
        tr, tg, tb, ta = FacePaint.GetColorTop()
        br, bg, bb, ba = FacePaint.GetColorBottom()
    else
        local top, bot = bordercolor.top, bordercolor.bot
        tr, tg, tb, ta = top.r, top.g, top.b, top.a
        br, bg, bb, ba = bot.r, bot.g, bot.b, bot.a
    end
    object:SetGradientAlpha("VERTICAL", br, bg, bb, ba, tr, tg, tb, ta)
end

The extra checks for "if FacePaint and FacePaint.GetColorTop" just ensure things don't break when someone uses an older version of FacePaint.

[Aprikot]

Originally Posted by Saiket I suggest creating "getter" functions instead though, to simplify how other addons interact with FacePaint. Basically, instead of requiring other mods to reach into the guts of FacePaint to get color values, FacePaint could provide a few standard, reliable functions to return color settings. If it had two functions, FacePaint.GetColorTop and FacePaint.GetColorBottom, they could handle the logic of when colors are flipped, or if class coloring is enabled, or any other feature FacePaint gets in the future.

This is an excellent idea (soon to be adopted ).

Regarding namespace: at the top of the code you posted I'm having trouble understanding how a table is being created:

local ns = select(2, ...)
FacePaint = ns
 
 -- as opposed to 
FacePaint = {}
 
-- or am I to make a table after?
local ns = select(2, ...)
FacePaint = ns
FacePaint.Test = {
-- this is a table called "Test" in the "FacePaint" namespace?
}

Also, with a namespace of "FacePaint" defined, does this mean that other AddOns could then reference FacePaint's tables, variables, and functions by name as FacePaint."name", or is this only intended for cross-file sharing within the FacePaint AddOn directory?

[Starinnia]

Blizzard passes two variables in through the vararg for every file in your addon. The first is the name of the addon (as defined by the containing folder) and the second is a table that is unique to the addon.

So the code

local ns = select(2, ...)
FacePaint = ns

takes the table Blizzard provides and uses it for your addon's namespace. The namespace is then given the global name of FacePaint so it can be accessed externally from your addon.

[Aprikot]

Originally Posted by Starinnia Blizzard passes two variables in through the vararg for every file in your addon. The first is the name of the addon (as defined by the containing folder) and the second is a table that is unique to the addon. So the code local ns = select(2, ...) FacePaint = ns takes the table Blizzard provides and uses it for your addon's namespace. The namespace is then given the global name of FacePaint so it can be accessed externally from your addon.

Ahhhh...all is made clear, thanks!

[Torhal]

Originally Posted by Starinnia Blizzard passes two variables in through the vararg for every file in your addon. The first is the name of the addon (as defined by the containing folder) and the second is a table that is unique to the addon. So the code local ns = select(2, ...) FacePaint = ns takes the table Blizzard provides and uses it for your addon's namespace. The namespace is then given the global name of FacePaint so it can be accessed externally from your addon.

Though that defeats the whole point of the AddOn-local table - at that rate, you could just declare a global variable and assign an empty table to it.

[Saiket]

Originally Posted by Torhal Though that defeats the whole point of the AddOn-local table - at that rate, you could just declare a global variable and assign an empty table to it.

And waste a perfectly good table?!