Return value LEAK

07-13-2007, 06:58 AM

Return values leak sometimes, but only if you return local variables.

For example:

Code:

function CreatePeasantAtCenter takes nothing returns unit
    local unit u = CreateUnit(Player(0), 'phea', 0, 0, 0)
    return u
endfunction

to fix this you would do:

Code:

function CreatePeasantAtCenter takes nothing returns unit
   return CreateUnit(Player(0), 'phea', 0, 0, 0)
endfunction

these examples are fairly simple so people might think ok
I will just never use a local to hold return value,
but it does not work that way.

Sometimes you need a variable to hold your return value before
you return it, (some processing needed before return)

One example is a function that creates a struct and fills it with some data.

Another example is infamous leak in blizzards GetUnitsOfPlayerAll()
Here is what blizzard did:

Code:

function GetUnitsOfPlayerAll takes player whichPlayer returns group
    return GetUnitsOfPlayerMatching(whichPlayer, null)
endfunction

// witch calls this:

function GetUnitsOfPlayerMatching takes player whichPlayer, boolexpr filter returns group
    local group g = CreateGroup()
    call GroupEnumUnitsOfPlayer(g, whichPlayer, filter)
    call DestroyBoolExpr(filter)
    return g                             // <---- oh yeah return LEAK
endfunction

// blizzard could have avoided this by using globals like they usually do

function GetUnitsOfPlayerMatching takes player whichPlayer, boolexpr filter returns group
    set bj_lastCreatedGroup = CreateGroup()
    call GroupEnumUnitsOfPlayer(bj_lastCreatedGroup, whichPlayer, filter)
    call DestroyBoolExpr(filter)
    return bj_lastCreatedGroup      // <---- no more leak, bj_ is a global
endfunction

// but for some reason they forgot to do it here

Bottom line:
if you have a function that creates something,
store the value in your custom global
or use bj_s if you think appropriate
just stay away from locals.

07-13-2007, 07:16 AM

DioD

Code:

function CreatePeasantAtCenter takes nothing returns unit
    local unit u = CreateUnit(Player(0), 'phea', 0, 0, 0)
    return u
endfunction

leaks local's pointer (minor)

Code:

function CreatePeasantAtCenter takes nothing returns unit
   return CreateUnit(Player(0), 'phea', 0, 0, 0)
endfunction

leak nothing, but there is BJ like function == useless

Code:

function CreatePeasantAtCenter takes nothing returns nohing
   set EX_LAST_UNIT = CreateUnit(Player(0), 'phea', 0, 0, 0)
endfunction

Some times we need to "Execute" other functions, best solution of the problem is temp global, blizzard used it, but there is better way...

Code:

function CreatePeasantAtCenter takes unit Null returns unit
   set Null = CreateUnit(Player(0), 'phea', 0, 0, 0)
   return Null
endfunction

To fix we allocate parameter value from function, soo we can do anything we want with created unit

function do any
//we must pass value, i used pregenerated handles to avoid passing nulls...
call Kill(CreatePeasantAtCenter(NULL))
endfunction

07-13-2007, 07:31 AM

cohadar

Quote:

Originally Posted by DioD

[code]
>> leaks local's pointer (minor)

>> leak nothing, but there is BJ like function == useless

>> To fix we allocate parameter value from function, soo we can do anything we want with created unit
endfunction

There is no such thing as i minor leak,
if you put it in a periodic timer it very soon becomes major one.

BJ like functions are not useless,
there is a thing called over-optimization
(garbling of code with no real performance purpose)

Adding return arguments is a good idea. blizzard uses it in natives
example : GroupEnumUnitsOfPlayer

but you must mark them properly and change the function name
so the user knows they are return parameters and not just some rubbish.

Also bj_ like globals are sometimes really necessary so this method is not
"better" it is just different.

07-13-2007, 08:29 AM

DioD

globals is much more better.

if used correctly there is no need of locals at all.

i used this with SCV, making CreateTimerEx() function that create timer and place it into global.

function
if Ex_Crushed != null then
call Echo("Thread Crush at " + Ex_Crushed)
call Echo(I2S(Ex_Last_Integer))
endif
set Ex_Crushed = "This function name"
call CreateTimerEx() //timer created and stored in global Ex_Last_Handle
call Ha2StEx() //store string and integer value of last handle
//Ha-Handle St-String Ex-Extended eg stores result in globals
call StoreInteger(Ex_Core,Ex_Last_String,"handleid",Ex_Last_Integer)
set Ex_Crushed = null
endfunction

by globals there is no need of locals or nullifications.
also you may easy track thread crushes by globals and track data used by functions cause it is not gone or nulled.

and minor leak is pointer
major is handle, like locations of timers

07-13-2007, 09:10 AM

cohadar

Quote:

Originally Posted by DioD

globals is much more better.

if used correctly there is no need of locals at all.

by globals there is no need of locals or nullifications.

also you may easy track thread crushes by globals and track data used by functions cause it is not gone or nulled.

and minor leak is pointer
major is handle, like locations of timers

I agree with you completely,
also would like to add that globals that are used as return values
should be private inside a scope or library to avoid synchronization problems.

(pointers are from systems that don't have garbage collector, like C/C++)

so you were saying:
minor = reference (or handle)
major = in game object that handle is referencing

very interesting notation, I am just not sure of this:

Lets assume timers object takes 30 bytes of memory.
and we have a handle that points to that timer.

If we destroy the timer does the game simply stops the timer from working
(and optionally zeroes those 30 bytes) or it actually frees those 30 bytes.

Or maybe it waits for handle to be free also to free those 30 bytes.

Any comparison of how this is done in C# or Java is invalid here
because JASS has completely berserk implementation.

So basically I would need to make a test map to check this out.
(currently at work, when I get home)

If you have that test already I would like to see it.

PS: it nice talking to someone who is versatile in this stuff.

07-13-2007, 09:48 AM

Toadcop

btw... it's bullshit ^^ cause try to do...

function test takes nothing returns nothing
local unit u=some_global_unit
endfunction

this function will absolutely not leak anything. the fault is bad coders.

Quote:

leaks local's pointer

it's a fairy tale for lamers.

your example would simple leak cause the handle id will be "used forever" and war3 would need to get some new one == "leak" but nit really.
it's a amazing old story you would better look what you are writting in your algorithms than to hunt every idiotical leak. <_<

// sorry it must be ^^ i hate disinformations...

07-13-2007, 10:49 AM

cohadar

Quote:

Originally Posted by Toadcop

Actually it will cause a memory loss
(I avoid the term leak here because you seem to think that leak is only non destroying objects)

If you make a periodic event in witch you do this:

Code:

// periodic or a loop

set some_global_unit = CreateUnit()
call test()
call RemoveUnit(some_global_unit)

// and modify test function this way
function test takes nothing returns unit
    local unit u=some_global_unit
    return u
endfunction

// we are talking about return leaks so don't plant bad examples please

It will leak - big time.

So I think you just stepped into some bullshit,
better clean your shoes.

07-13-2007, 11:25 AM

Rising_Dusk

Pointer leaks are 4 byte references.
Leaking these are as "minor" as it comes with leaks.

JASS:

function Rawr_1 takes nothing returns location
    local location l = Location(0,0)
    return Location(0,0)
endfunction

Which is the worse leak? The pointer? Or the location?
An entire map can go through 100% of its code and NEVER reset a pointer and it will not lag (I've tried it with both of my maps).
However, leaking a couple dozen locations every minute or so really adds up and can cause huge buildup of fps loss.

Location leaks are just my example, leaking groups, rects, etc. yields the same result.
The pointers really don't matter, so yes they are a very "minor" leak.

07-13-2007, 12:18 PM

Toadcop

cohadar <_< in your case units are leaking.

omg... ok i will test it AGAIN...

well im was alomst correct ! the "new" feature is what if some function returns value and you don't use it (for example call SomeWhatReturnsUnit() will leak and set xtmp=SomeWhatReturnsUnit() will NOT leak maybe there some stack cleaning problem i don't know but it's a fact ! + the leaks are VERY minor ! )

i have called this code in a 0.001 period and NO leaks ! but if i was calling simple this functions (not using them as a value) i get some minor but constant leaks. the problem is return with out return it works flawless !

Code:

function XXXXX takes nothing returns unit
    local unit u=gg_unit_Hblm_0001 // it's a global unit
    return u
endfunction

globals
    unit uuuxxx=null
endglobals

function Trig_per_Actions takes nothing returns nothing
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()

    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()

    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()

    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()

    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
    set uuuxxx=XXXXX()
endfunction

well but i think also this leaks are not worth to be mentioned cause ~50K calls and you will get 4 KB less memory.

conclusion to much panic =\
// btw i have thinked about this long ago what some func returns value and what happens if you don't "use" it... so the answer: it will leak. but extreme minor.

07-13-2007, 12:34 PM	#10
DioD	reference to static unit is always same, we talk about dinamic triggers\actions with reference to different handles.

07-13-2007, 12:48 PM	#11
grim001	Leaking a 4 byte pointer is "not a big deal" but, the problem is that unless you null all references to a handle its handle ID will never be recycled that causes the handle ID to continuously go up, and WC3 seems to slow down the higher the handle ID gets, whether you are leaking anything else or not. It seems that a high handle ID inherently slows down WC3.

07-13-2007, 12:49 PM

#12

Vexorian

Quote:

JASS:

function test takes nothing returns nothing
local unit u=some_global_unit
endfunction

this function will absolutely not leak anything. the fault is bad coders.

The fault here is bad testers.

If you later removed the unit pointed by the global and set the global to null or assigned something else to it, the local instances in that function would still point towards the unit thus there will be a leak of 4 bytes.

Quote:

One example is a function that creates a struct and fills it with some data.

These leaks are only caused by the faulty ref counter from blizzard, and thus they only affect handles, not integers, so structs are "safe"

07-13-2007, 01:39 PM

#13

cohadar

Since we are talking about structs I was thinking something like this:

If you have some handles in a struct, for example

Code:

struct data
    unit u
    timer t
    location l
endstruct

is there any point in nulling those handles in onDestroy method?
(I saw someone somewhere recommending it)

I believe not because number of handles in struct is limited to 8192
so even if it leaks them all it is still only 32kb of memory = unnoticeable.

07-13-2007, 03:44 PM

#14

Vexorian

yes and no.

JASS:

struct data
    unit u
    timer t
    location l
endstruct

If it was

JASS:

struct data
    unit u=null
    timer t=null
    location l=null
endstruct

which is logical since it is better to have stuff initialized correctly.

then .allocate() (or .create()) would set stuff to null automatically everytime it is called.

If you did d.u=GetTriggerUnit() and later d.destroy() and didn't set d.u to null, since d was destroyed .create will eventually recycle the id and assign null again to d.u , so there isn't really any problem there as long as you do use .destroy and .create

07-13-2007, 04:03 PM

#15

DioD

i posted SDARS somewhere, this system uses slot table to generate ID's for structs\units and other staff, im not finished it but currently it allocate "used" and "free" sector in some array, there is no used slots inside free sector and if slot marked free, 2 cycles later it will be reused.
reuse will replace values inside arrays and no leaks will happen.

Basicaly it was created for "return bug free" attach to minimize loop seek, later i used it to generate "second handle" for units.
Currently it allocate slots stable and can be used to generate clusters, soon arrays.

Vjass structs is no perfect, but fast and simple.
Id generation uses single linked array list and 2 globals.

I hate vjass and structs only for one thing - its hide real code from me and generate its own staff.
this is problem of any high level programming, and its much more harder do debug\improve.

Every struct generate parralel arrays and use them in direct order if no arrays free, reverse order if any struct were destroyed.

Linked list array returns slots in reverse order eg (3.2.1) from last free to first free, then will jump to use "new" slots.

i tested algoritm hard.
if slot 6 free,
zero will be pushed to array slot 6 and external handle will got 6 as its value.
if slot 99 free
6 will be pushed to array slot 99 and external handle will got 99 as its value.

next use will return 99 and 6 will pop to external handle

if any other slot free if will stay before 6 and this id will stuck for a long time.
dinamic usage may cause some slots to stuck permanently if spell or system used much and is not instant, eg buffs auras and other staff.