• 🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
  • It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!

Things That Leak

Level 16
Joined
May 2, 2011
Messages
1,345
Lets have a look at this little trigger.

  • Hard Extra Defense
    • Events
    • Conditions
    • Actions
  • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • (Number of living Grunt units owned by Player 1 (Red)) Less than 4
    • (Number of living Barracks units owned by Player 1 (Red)) Greater than 0
  • Then - Actions
Now to fix the leaking trigger above, I will write it like this:
  • Hard Extra Defense
    • Events
    • Conditions
    • Actions
      • Set GroupTemp01 = (Units owned by Player 1 (Red) matching (((Matching unit) is alive) Equal to True))
      • Set GroupTemp02 = (Units owned by Player 1 (Red) matching (((Matching unit) is alive) Equal to True))
      • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
        • If - Conditions
          • (Number of units in GroupTemp01) Less than 4
          • (Number of units in GroupTemp02) Greater than 0
        • Then - Actions
      • Custom script: call DestroyGroup(udg_GroupTemp01)
      • Custom script: call DestroyGroup(udg_GroupTemp02)
is that it? or what I did was totally unnecessary anyway?

@MyPad Post confused me o_O
so it leaks or not?
 
The first example will leak. However, the second will not leak objects, since you destroy the group handles.

Condition or no condition, creating a group means that you have to have the responsibility of destroying it.


If you'd like to get more technical, there is a bug that causes the handle stack to permanently increase, despite the successful elimination of handle objects (in this case, groups). It presents itself when a locally declared variable is passed as a return value in a function. Notwithstanding, the functions in GUI about unit groups all return the locally declared group handle.

It was present before, and might still be present now.
 
Level 38
Joined
Feb 27, 2007
Messages
4,951
@map designer you can also automatically clean the group leak by doing Custom script: set bj_wantDestroyGroup = true right before the line that creates the group. That line tells the game to automatically destroy the next group it makes once it's done with it.

Unfortunately that will not work when you have 2 different groups created in the same if-block, since you can't put CS lines in there in GUI. It will work for just one, though. Also don't will-nilly set that variable unless you know FOR SURE you are about to create a group, since setting it and not making a group would cause the game to destroy whichever group it next makes, likely in a totally unrelated trigger.
 
Level 16
Joined
May 2, 2011
Messages
1,345
@map designer you can also automatically clean the group leak by doing Custom script: set bj_wantDestroyGroup = true right before the line that creates the group. That line tells the game to automatically destroy the next group it makes once it's done with it.

Unfortunately that will not work when you have 2 different groups created in the same if-block, since you can't put CS lines in there in GUI. It will work for just one, though. Also don't will-nilly set that variable unless you know FOR SURE you are about to create a group, since setting it and not making a group would cause the game to destroy whichever group it next makes, likely in a totally unrelated trigger.
hmmmm

I thought that dynamic function will automatically destroy any group created by this trigger at the end of this trigger.

this raised the question for me though: when does this function destroy the group? at the end of this trigger? or when? if it was destroyed pre-maturely it would miss up the trigger wouldnt it?
 
Level 38
Joined
Feb 27, 2007
Messages
4,951
Every unit group action uses ForGroupBJ() internally. The destroy happens right at the end of the function call:
JASS:
function ForGroupBJ takes group whichGroup, code callback returns nothing
   // If the user wants the group destroyed, remember that fact and clear
   // the flag, in case it is used again in the callback.
   local boolean wantDestroy = bj_wantDestroyGroup
   set bj_wantDestroyGroup = false

   call ForGroup(whichGroup, callback)

   // If the user wants the group destroyed, do so now.
   if (wantDestroy) then
       call DestroyGroup(whichGroup)
   endif
endfunction

The way it stores the value of bj_wantDestroyGroup locally before calling ForGroup() itself allows you to nest group picks and they'll all be properly destroyed:

  • Custom script: set bj_wantDestroyGroup = true
  • Unit Group - Pick every unit in...
    • Loop - Actions
      • Custom script: set bj_wantDestroyGroup = true
      • Unit Group - Pick every unit in...
        • Loop - Actions
          • Custom script: set bj_wantDestroyGroup = true
          • Unit Group - Pick every unit in...
            • Loop - Actions
              • -------- ad infinitum --------
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
what I mean sir is,, I thought key is like an array of a variable isnt it?
There's a parent key, and a child key. Each parent represents an address which can store child keys with indices as high as integers allow.

So really, each parent key is a single array with "unlimited" slots but one which doesn't need to initialize all previous array indices in between slots. That's the difference between a hash table and an array.

So really I just want to make sure we're on the same page, cause you're not making too much sense so far.
 
Level 19
Joined
Dec 12, 2010
Messages
2,069
I highly discourage that. I recommend only storing one variable per parent/child key combination. If there is any data to show what I'm saying can be disregarded (ie. if they have done any tests) feel free to show me up here.
actually you'd better reduce amount of parents to lowest possible, thats overheat right there. Better to have Save*(HT,0,spellid,callback) rather than Save*(HT,spellid,0,callback)
 
Level 26
Joined
Aug 18, 2009
Messages
4,097
Sir can I use same key in each variable types in hashing to one parent and table?

At least handles are not disjunct. There is still the return bug substitute floating around where you can store an integer as fogstate and load it as another handle type. There is also the RemoveSavedHandle native, which hints at that. But often, you can limit yourself to storing integer indexes in hashtables and then use them in arrays. Or you could just use multiple hashtables.

actually you'd better reduce amount of parents to lowest possible, thats overheat right there. Better to have Save*(HT,0,spellid,callback) rather than Save*(HT,spellid,0,callback)

How so?

Good info there to add for him. Though I still would like to know what he's talking about.

JC Helas asked if e.g. SaveInteger(table1, parentKey1, key1, <integer>) and SaveUnitHandle(table2, parentKey2, key2, <unit>) where table1==table2, parentKey1==parentKey2, key1==key2 is viable, probably if they are disjunct as I responded to.
 
Last edited:
Level 19
Joined
Dec 12, 2010
Messages
2,069
Maybe technicall they are identical in terms of storage usage, but remember one patch where blizzard broke hashtables by limiting parents count to 1024 or so? Within engine parents are being counted and stored, RemoveSavedX doesn't deduct parent counter, FlushChildHashtable() does although. I dont know where this counter is used and what purpose he serves, but why would you increase this counter with no good reason to? Thats my logic.
 
Level 26
Joined
Aug 18, 2009
Messages
4,097
Maybe technicall they are identical in terms of storage usage, but remember one patch where blizzard broke hashtables by limiting parents count to 1024 or so? Within engine parents are being counted and stored, RemoveSavedX doesn't deduct parent counter, FlushChildHashtable() does although. I dont know where this counter is used and what purpose he serves, but why would you increase this counter with no good reason to? Thats my logic.

Never heard of it. I have no insight of the internals. When you say there's a counter, that seems marginal in terms of performance. My idea is to discriminate it by the FlushChildHashtable function, children are mapped to a parent, and this function makes it easy to erase everything in one swoop. Is the limit still in existence?
 
Level 19
Joined
Dec 12, 2010
Messages
2,069
the counter itself means nothing, but as I said there might be inner stuff bound to it. HT is already linked list, MAYBE this counter used for something else what matter, maybe not. This arg swap costs 0 time for mapmaker so should be respected as long as it fits codestyle
 
Level 19
Joined
Dec 12, 2010
Messages
2,069
What if I will do this way

unit u = dummy
integer i = 1
real r = 1.00
integer id = GetHandleId(u)
SaveInteger(table,id,0,i)
SaveReal(table,id,0,r)

look I use 0 key in different native. is this leak?
read documentary, hashtable can save 1 type per every cell, int/real/bool/handle/string, no problems. only handles can leak of those
 
Level 15
Joined
Jan 16, 2009
Messages
109
hey, guys. need to know is this function leaks "uC" cause i'm not nullifying it?

JASS:
function GroupGetClosestUnit takes unit whichUnit, group whichGroup returns unit
    local integer i = 0
    local unit u = FirstOfGroup(whichGroup)
    local unit uC
    local unit array uG
    local real x = GetUnitX(whichUnit)
    local real y = GetUnitY(whichUnit)
    local real xT = GetUnitX(u)
    local real yT = GetUnitY(u)
    local real dist
    local real distC = SquareRoot((x - xT) * (x - xT) + (y - yT) * (y - yT))
    loop
        set u = FirstOfGroup(whichGroup)
        exitwhen u == null
        set xT = GetUnitX(u)
        set yT = GetUnitY(u)
        set dist = SquareRoot((x - xT) * (x - xT) + (y - yT) * (y - yT))
        if dist < distC then
            set uC = u
        endif
        set uG[i] = u
        set i = i + 1
        call GroupRemoveUnit(whichGroup, u)
    endloop
    loop
        exitwhen i < 0
        call GroupAddUnit(whichGroup, uG[i])
        set uG[i] = null
        set i = i - 1
    endloop
    set u = null
    return uC
endfunction
 

Dr Super Good

Spell Reviewer
Level 63
Joined
Jan 18, 2005
Messages
27,178
hey, guys. need to know is this function leaks "uC" cause i'm not nullifying it?
Yes it causes the handle index referenced by uC at the time of return to have a leaked reference count and hence when that unit is removed the handle index assigned to it cannot ever be recycled.

Solution is to use either a function parameter or a global variable to temporarily hold the value while the local is nulled and then return the value.
 
Level 15
Joined
Jan 16, 2009
Messages
109
Yes it causes the handle index referenced by uC at the time of return to have a leaked reference count and hence when that unit is removed the handle index assigned to it cannot ever be recycled.

Solution is to use either a function parameter or a global variable to temporarily hold the value while the local is nulled and then return the value.
so now its basically fixed? i can simply make uTemp parameter equal to "null".
JASS:
function GroupGetClosestUnit takes unit whichUnit, group whichGroup, unit uTemp returns unit
    local integer i = 0
    local unit u
    local unit uC = FirstOfGroup(whichGroup)
    local unit array uG
    local real x = GetUnitX(whichUnit)
    local real y = GetUnitY(whichUnit)
    local real xT = GetUnitX(uC)
    local real yT = GetUnitY(uC)
    local real dist
    local real distC = SquareRoot((x - xT) * (x - xT) + (y - yT) * (y - yT))
    loop
        set u = FirstOfGroup(whichGroup)
        exitwhen u == null
        set xT = GetUnitX(u)
        set yT = GetUnitY(u)
        set dist = SquareRoot((x - xT) * (x - xT) + (y - yT) * (y - yT))
        if dist < distC then
            set uC = u
        endif
        set uG[i] = u
        set i = i + 1
        call GroupRemoveUnit(whichGroup, u)
    endloop
    loop
        exitwhen i < 0
        call GroupAddUnit(whichGroup, uG[i])
        set uG[i] = null
        set i = i - 1
    endloop
    set uTemp = uC
    set u = null
    set uC = null
    return uTemp
endfunction
 
You for sure can use it like this, I just would personally not like to give an extra parameter for counter a ref leak. As usage seems becomes worse.

Using a global would be maybe nicer, as it would change nothing for code usage. Some use Blizzard globals like bj_lastCreatedUnit, but using a seperated variable is better, as it can't come unwilligly in conflict with other's code. (if some GUI user in the example would ever work with "last created unit")

But that's also not really a "perfect" solution maybe. You also might to consider risking the leak, if you're too lazy for the global. ^^
 
Level 16
Joined
May 2, 2011
Messages
1,345
Hello,
this conition for trigger will leak right?
Event-
Unit owned by player 11 dies.
Conditions -
(Number of units in (Units owned by Player 11 (Dark Green))) Equal to 0

what about this condition
(Count structures controlled by Player 12 (Brown) (Exclude incomplete structures)) Equal to 0


and to fix it, I better move this condition into an if statement right? i.e. set Q equal number of units owned by etc
 
Last edited:

Dr Super Good

Spell Reviewer
Level 63
Joined
Jan 18, 2005
Messages
27,178
Hello,
this conition for trigger will leak right?
Event-
Unit owned by player 11 dies.
Conditions -
(Number of units in (Units owned by Player 11 (Dark Green))) Equal to 0
Yes it leaks a group and even if that leak were fixed it will leak a handle ID.
and to fix it, I better move this condition into an if statement right? i.e. set Q equal number of units owned by etc
To fix the group leak you need to do what IcemanBo describes and move the test into an if block in the actions. This allows one to assign a unit group variable before performing the test and use DestroyGroup on it after the test.

Fixing the handle ID leak is harder. Requiring either a custom script call to a fixed version of "Units owned by" function or importing a fixed blizzard.j file. Importing a fixed blizzard.j file is not recommended as it might negatively effect a maps forwards compatibility with future versions of Warcraft III if Blizzard changes anything in that file.
what about this condition
(Count structures controlled by Player 12 (Brown) (Exclude incomplete structures)) Equal to 0
This does not leak as far as I can tell.
 
Level 16
Joined
May 2, 2011
Messages
1,345
That leaks 3 locations. Check the "Triggering tips" link in my signature for more information.
sorry,

I meant if I saved the 2 points in a variable already, I will destroy these two points later. again, what was the 3rd thing that would leak?


so, here is the trigger

  • IncenerationCast
    • Events
      • Unit - A unit Starts the effect of an ability
    • Conditions
      • (Ability being cast) Equal to Channel (Trigger Spell)
    • Actions
      • Quest - Display to (All players) the Quest Update message: SPELL HAS BEEN CAST
      • Set Temp_target1 = (Target unit of ability being cast)
      • Set Temp_Point1 = (Position of (Triggering unit))
      • Set Temp_Point2 = (Position of Temp_target1)
      • Unit - Create 1 Acid (Caster) for Player 1 (Red) at Temp_Point1 facing (Angle from Temp_Point1 to Temp_Point2) degrees
      • Set Temp_Caster2 = (Last created unit)
      • Unit - Order Temp_Caster2 to Neutral Alchemist - Acid Bomb Temp_target1
      • Unit - Create 1 Fire Trigger (Caster) for Player 1 (Red) at Temp_Point1 facing (Angle from Temp_Point1 to Temp_Point2) degrees
      • Set Temp_Caster1 = (Last created unit)
      • Unit - Order Temp_Caster1 to Night Elf Warden - Shadow Strike Temp_target1
      • Unit - Remove Temp_Caster2 from the game
      • Custom script: call RemoveLocation(udg_Temp_Point1)
      • Custom script: call RemoveLocation(udg_Temp_Point2)
 
Last edited:
is (angle between 2 points) leaking?
That leaks 3 locations. Check the "Triggering tips" link in my signature for more information.
I meant if I saved the 2 points in a variable already, I will destroy these two points later. again, what was the 3rd thing that would leak?

It's correct that you don't leak if you store the both locations into variables and remove them after usage. A third location or something else isn't used that could leak, here's the code behind:
JASS:
function AngleBetweenPoints takes location locA, location locB returns real
    return bj_RADTODEG * Atan2(GetLocationY(locB) - GetLocationY(locA), GetLocationX(locB) - GetLocationX(locA))
endfunction

or in general, does any arithmetic operation leak if they are not stored into a variable which then will be destroyed?
No, not really. It's just operations in the end, like 1+1, no object is created. For some background in general Memory Leaks.
 
Level 16
Joined
May 2, 2011
Messages
1,345
It's correct that you don't leak if you store the both locations into variables and remove them after usage. A third location or something else isn't used that could leak, here's the code behind:
JASS:
function AngleBetweenPoints takes location locA, location locB returns real
    return bj_RADTODEG * Atan2(GetLocationY(locB) - GetLocationY(locA), GetLocationX(locB) - GetLocationX(locA))
endfunction


No, not really. It's just operations in the end, like 1+1, no object is created. For some background in general Memory Leaks.
Does this mean my code is just fine? or is it better if I stored the angle in a variable Temp_angle? (I did update the trigger just now since I forgot to remove Caster2)
 
Last edited:
It is just fine as it is.
Keep in mind, to make the calculation, you make a function call, and some maths operation. If you need this angle multiple times, then you might use a temp variable. It's basically also easier to read good variable names in code than reading all functions somewhere in between. But still, this has nothing todo with a leak.
 
Level 16
Joined
May 2, 2011
Messages
1,345
It is just fine as it is.
Keep in mind, to make the calculation, you make a function call, and some maths operation. If you need this angle multiple times, then you might use a temp variable. It's basically also easier to read good variable names in code than reading all functions somewhere in between. But still, this has nothing todo with a leak.
Thanks!

the 3rd thing that would leak still bugs me though. what was that extra thing which would leak Had I not saved these variables?
 
The region should be destroyed unless it's one that you've created with the editor.

This is fine:
  • Destructible - Pick every destructible in MyRegion <gen> and do (Destructible - Kill (Picked destructible))
This leaks a region:
  • Destructible - Pick every destructible in (Region(0.00, 0.00, 0.00, 0.00)) and do (Destructible - Kill (Picked destructible))
Can be fixed like this:
  • Custom script: set udg_TempRegion = Rect(0, 0, 0, 0)
  • Destructible - Pick every destructible in TempRegion and do (Destructible - Kill (Picked destructible))
  • Custom script: call RemoveRect(udg_TempRegion)
  • Set TempRegion = No region
 
Last edited:
JASS:
local unit u

loop
    set u = FirstOfGroup(grp)
    exitwhen u == null
    call GroupRemoveUnit(grp, u)
endloop

Does the above code still leak a reference if we don't set u = null after the loop? I.e. is null still considered a reference if it's actually a null agent and not a plain null? Not sure if I explained that well.
 
No it's no leak, null == null.
null is also only a part in memory, but a special one for default. One does not have any rights to write in this memory, one only can read it. So always when a function should return some object for example, but can't return any valid object for what ever reason (for example there is no FirstOfGroup() anymore), it can use the return null possibility (and should do). So just always when "null" is returned it does mean the function can't return valid values, and there's no further steps in difference of "null". In the end it's only important the agents point on some other memory not equal the object's memory it pointed to, and null is such other part in memory.
 
It's not very the same, yes.

When you have "" as string, it is a true string object. It has no chars in it, but you can do everything you want with it, like you can with any other strings. It can call functionality.
When you try to work with null, the string functionality can't be used. It's no real object, so concatenating (which is just a function in some way for the string object) isn't working.

But also some test cases:

case 1:
set s = null + "text" -> compiler error. (expected)

Next, this is more or less, too, as expected, case 2:
JASS:
function nullAsString takes nothing returns string
    local string s // just for no inline
    return null
endfunction

local string s = nullAsString() + "test"
-> sums up to (null) , no valid string.

case 3 .. local string s = "test" + nullAsString() -> prints "test"
(at least from my theory above this is a bit unexpected)

And also, case 4:
JASS:
local string s = null
set s = s + "test"
-> printes "test"
(might be unexpected like case 3, because null can not have functionality)

===

Both last cases would not work in normal c++, for example.

One might argue case3 works over case2 because the first string is actually always the string which actually calls the functionality, so the concatenate function, and so it is valid. First string is the calling object. And then there are internal checks for the second parameter, if the string to add is null, which will result in like adding an empty string/just nothing.

Case 4, there is maybe some implicit string conversion directly done here, when assining null to a string variable, local string s = null. So from then it's simply seen as empty string "".

===

So when having a conrete string variable, set to null, it may act as "", while working directly with null doesn't work with string concatenation.
Working with functions that return null (without being stored into a string variable) does not work for calling string concatenation as first parameter, but it will not cause an invalid string, when given as second parameter.

Basically as long as the string is only input for some function that jass vm controls, and is not the calling object itself like in string concatenation, null seems always be reacted same like empty string "". It's most likely some jass comfort that it is like this, checking for null at many places to keep nicer usage.
 
Last edited:
Top