Does this leak? (Jass damage trigger)

[afisakov]

My tower defense map starts encountering serious lag toward end of game, and I cannot tell f from memory leaks or just because of shear # of towers. This does not happen on easy mode so I doubt it is from enemy spawn or pathing triggers.
This trigger intended to deal damage in 2 areas (full and half radius) to target attacked by tower, and runs very often so I want to make sure it is leak-proof.
I removed some redundancies to make it easier to see and cleaned up all the leaks I knew of.

Spoiler

function CD_atk takes unit u returns nothing
  call GroupAddUnitSimple(u, udg_CoolingDown)
  call PolledWait(0.3)
  call GroupRemoveUnitSimple(u, udg_CoolingDown)
endfunction

function Trig_Thunder_Conditions takes nothing returns boolean
local integer uid=GetUnitTypeId(GetAttacker())
local integer Lp1=1
if IsUnitInGroup(GetAttacker(),udg_CoolingDown) then
   return false
endif
loop
exitwhen Lp1>15
if uid == udg_Dmg_tower[Lp1] then
   return true
endif
set Lp1=Lp1+1
endloop
   return false
endfunction

function gendenemy takes nothing returns boolean
if IsUnitEnemy(GetFilterUnit(),GetOwningPlayer(udg_attacker)) and GetUnitState(GetFilterUnit(),UNIT_STATE_LIFE)>0.405 then
   call UnitDamageTarget(udg_attacker,GetFilterUnit(),udg_damage,true,false,udg_atktype,DAMAGE_TYPE_NORMAL,WEAPON_TYPE_WHOKNOWS)
endif
return false
endfunction

function Trig_Thunder_Actions takes nothing returns nothing
local location tp1
local group tg
local real cx
local integer uid
local integer pn
set udg_attacker=GetAttacker()
set uid=GetUnitTypeId(GetAttacker())
set pn=1+GetPlayerId(GetOwningPlayer(udg_attacker))
   set tp1=GetUnitLoc(GetTriggerUnit())
   if ( uid == 'o00K' ) then
     set udg_damage=GetRandomInt(200,550)*udg_abp[pn]
     set cx=120*udg_abp[pn]
     set udg_atktype = ATTACK_TYPE_PIERCE
   elseif ( uid == 'o01G' ) then
     set udg_damage=2000*udg_abp[pn]
     set cx=300*udg_abp[pn]
     set udg_atktype = ATTACK_TYPE_CHAOS
     call AddSpecialEffectLocBJ( tp1, "Abilities\\Spells\\Undead\\AnimateDead\\AnimateDeadTarget.mdl" )
     call DestroyEffect(GetLastCreatedEffectBJ())
   -more elseif's of similar type
   endif
   if (GetUnitAbilityLevel(udg_attacker,'B00B')>0) then
   set udg_damage=udg_damage*1.5*udg_abp[pn]
   endif
   set tg=GetUnitsInRangeOfLocMatching(cx*2.,tp1,Condition(function gendenemy))
   call DestroyGroup(tg)
   set tg=GetUnitsInRangeOfLocMatching(cx,tp1,Condition(function gendenemy))
   call DestroyGroup(tg)
   call RemoveLocation (tp1)
   call CD_atk(GetAttacker())
   set udg_attacker=null
   set tg=null
   set tp1=null
endfunction

//===========================================================================
function InitTrig_Thunder takes nothing returns nothing
   set gg_trg_Thunder = CreateTrigger()
   call TriggerRegisterAnyUnitEventBJ( gg_trg_Thunder, EVENT_PLAYER_UNIT_ATTACKED )
   call TriggerAddCondition( gg_trg_Thunder, Condition(function Trig_Thunder_Conditions) )
   call TriggerAddAction( gg_trg_Thunder, function Trig_Thunder_Actions )
endfunction

The part I am most worried about is

Spoiler

function CD_atk takes unit u returns nothing
  call GroupAddUnitSimple(u, udg_CoolingDown)
  call PolledWait(0.3)
  call GroupRemoveUnitSimple(u, udg_CoolingDown)
endfunction

needed to prevent domino-effect of wiping out a wave because trigger-damage kill does not use up unit attack- resulting in instant re-attack

 

[jonas]

Yeah, leaks. Condition creates a new object.

E.g.,

set tg=GetUnitsInRangeOfLocMatching(cx*2.,tp1,Condition(function gendenemy))
call DestroyGroup(tg)
set tg=GetUnitsInRangeOfLocMatching(cx,tp1,Condition(function gendenemy))

Instead, use a global boolexpr.

 

[afisakov]

ok, so is the problem with

set tg=GetUnitsInRangeOfLocMatching(cx*2.,tp1,Condition(function gendenemy))
call DestroyGroup(tg)

or

function gendenemy takes nothing returns boolean
if IsUnitEnemy(GetFilterUnit(),GetOwningPlayer(udg_attacker)) and GetUnitState(GetFilterUnit(),UNIT_STATE_LIFE)>0.405 then
call UnitDamageTarget(udg_attacker,GetFilterUnit(),udg_damage,true,false,udg_atktype,DAMAGE_TYPE_NORMAL,WEAPON_TYPE_WHOKNOWS)
endif
return false
endfunction

and how should I fix it.
I took my condition syntax from the GUI set variable/ unit group conversion, e.g.

set udg_TempGroup = GetUnitsInRectMatching(GetPlayableMapRect(), Condition(function Trig_Untitled_Trigger_001_Func001002002))

adjusting t fix point leak of course

 

[jonas]

Here's an example:

    boolexpr reselect = null;
    public function Reselect () {
        if (reselect == null)
            reselect = Filter(function reselectEnum);
           
        bj_lastCreatedGroup= CreateGroup();
        GroupEnumUnitsSelected(bj_lastCreatedGroup, GetCurrentPlayer(), reselect);
        DestroyGroup(bj_lastCreatedGroup);
    }

 

[afisakov]

Here's an example:

    boolexpr reselect = null;
    public function Reselect () {
        if (reselect == null)
            reselect = Filter(function reselectEnum);
          
        bj_lastCreatedGroup= CreateGroup();
        GroupEnumUnitsSelected(bj_lastCreatedGroup, GetCurrentPlayer(), reselect);
        DestroyGroup(bj_lastCreatedGroup);
    }

1) I can't really make sense of how to implement this, does it require a modified WE, like WEUnlimited or newgen?

2) Does this mean the GUI function "GetUnitsInRangeOfLocMatching" is inherently leaky, if used without custom JAss scripts (besides the well known call DestroyGroup and call RemoveLocation)?

3) Can I fix my leak by replacing with:

function gendenemy takes nothing returns nothing
if IsUnitEnemy(GetFilterUnit(),GetOwningPlayer(udg_attacker)) and GetUnitState(GetFilterUnit(),UNIT_STATE_LIFE)>0.405 then
call UnitDamageTarget(udg_attacker,GetFilterUnit(),udg_damage,true,false,udg_atktype,DAMAGE_TYPE_NORMAL,WEAPON_TYPE_WHOKNOWS)
endif
endfunction

set tg=GetUnitsInRangeOfLocAll(cx,tp1)
call ForGroupBJ(tg,function gendenemy)
call DestroyGroup(tg)

It does not seem as efficient as filtering the units as I make the group, but if it doesn't leak that would be worth it.
And until I understand what step made the original leak, I am not sure how to design a better way in regular world editor (I have been using "Convert to Custom text" to optimize the code further but still mostly work out of WE)

 

[jonas]

Alright, I made some checks and it turns out that Condition does not actually create new objects (the connectors like Or() etc, however, do).
Disregard my previous posts. I see no other possibilities of a leak in the code you have shown to us.

 

[afisakov]

ah, ok thanks for looking.
btw, is

if IsUnitEnemy(GetFilterUnit(),GetOwningPlayer(udg_attacker)) and GetUnitState(GetFilterUnit(),UNIT_STATE_LIFE)>0.405 then

a leak, or did "(the connectors like Or() etc, however, do)." only refer to the GUI ones like
GetBooleanAnd(a,b) ?

 

[jonas]

The functions here:

http://jass.sourceforge.net/doc/api/common_j-source.shtml#1049

 

[afisakov]

So these natives are safe to use then, correct?

Sorry if I am slow to get it, just trying to understand why one thing does or does not leak.
For instance, would "GetBooleanAnd" leak because it converts both Boolexpr to boolean values and saves them before returning?
Unfortunately thus far I have just been memorizing what leaks and how to fix it, still having a hard time wrapping my head around how to tell if new stuff (not group or point) would leak.
And this thread is the first time I realized boolexpr was different from boolean and tried to make sense of the word.

 

[jonas]

And this thread is the first time I realized boolexpr was different from boolean and tried to make sense of the word.

Yes, they are very different.
If you write a boolean expression like true and false, it will be evaluated in place (and become either true or false).
A boolexpr is not evaluated in place, but can be carried around and evaluated at a much later place.

For whether something leaks or not, you have to understand the memory model behind JASS.
There are two main areas for things to live: the heap and the stack.

You could think of the heap as a small city.
Whenever you create an object, it moves into a house in the city.
In order to access data of the object, you have to find that house. For that you need the address of the house: the handle.
Every time you create a new object, it needs a free house. In order for the city not to get overpopulated, you need to kick data out from a house when its no longer needed (Remove.../Destroy...).

Now there are two bad things that can happen.
A leak is when you create some data but you lose the address. The data will live in its house forever, since you can not find it and kick it out anymore.
A handle leak is when you kicked data out of its house, but some people might still have the address to that house. Before someone new can move in, all people need to forget that the house exists.

Local and global variables live, for the sake of argument, on the stack. String variables, integer variables, real variables, boolean variables, handle variables. Note that handle variables only contain the handle, i.e., the address of the data, not the data itself.
When a function completes, all the local variables are removed from the stack and the data is freed.

But here's a bug of the JASS engine: the game doesn't understand that when you remove a local handle variable from the stack, it destroys the address that the variable contained. It will believe that there is still someone who has the address to that house, and never give the address to someone else. That's why we explicitly tell the game to do so by nulling the variable, i.e., discarding the old address and getting a different one. It is interesting to note that the bug is not there with parameters, so you don't have to null parameters.

The only way to put data on the heap is with a native. Everything that creates a new thing leaks. GetBooleanAnd is not a native and doesn't use natives, so it can't leak. Or() is a native and returns a new entity, so it leaks. Condition apparently creates the same thing each time you call it on the same parameter:

function FALSE_FUNC takes nothing returns boolean
    return false
endfunction

function TTT takes nothing returns nothing
    if (Condition(function FALSE_FUNC) == Condition(function FALSE_FUNC)) then
        call BJDebugMsg("same")
    endif
endfunction

//===========================================================================
function InitTrig_Unbezeichneter_Ausl__ser_001 takes nothing returns nothing
    set gg_trg_Unbezeichneter_Ausl__ser_001 = CreateTrigger(  )
    call TriggerAddAction( gg_trg_Unbezeichneter_Ausl__ser_001, function TTT )
endfunction

Now you know.

 

[afisakov]

So a quick summary, created points,groups have to be removed/destroyed
local variables like unit/group/point/player have to be set=null as well
local real or integer do not, as far as I know, local string set="" instead

code like:

if A or B then

should not leak,
but

if Or(A,B) then

will.
Based on what you where saying earlier, will Not(A==B) leak?

P.S. Wow, you reply pretty fast. Much less latency between responses than my posts.
Thanks

 

[jonas]

A==B is a boolean, not a boolexpr. But indeed, Not() creates a new entity. You can easily check yourself with the following trigger:

scope testscope initializer onInit
function FALSE_FUNC takes nothing returns boolean
    return false
endfunction

function onInit takes nothing returns nothing
    if (Not(Condition(function FALSE_FUNC)) != Not(Condition(function FALSE_FUNC))) then
        call BJDebugMsg("different")
    endif
endfunction
endscope

 

[afisakov]

thank you